-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[VP][EVL] Support select instruction with EVL-vectorization #109614
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-llvm-transforms Author: LiqinWeng (LiqinWeng) ChangesFull diff: https://github.com/llvm/llvm-project/pull/109614.diff 7 Files Affected:
|
|
||
Value *Cond = InvarCond ? InvarCond : State.get(getCond(), 0); | ||
if (!isa<VectorType>(Cond->getType())) { | ||
Cond = BuilderIR.CreateVectorSplat(State.VF, Cond, "splat.cond"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you have a test for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't construct such a test case, but adding conditions may be safer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need a test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I tried to wirte such a test case, but it didn't sucess, so I will remove this condition for now.
790bbf6
to
eba26a7
Compare
@@ -1116,9 +1116,8 @@ void VPWidenSelectEVLRecipe::execute(VPTransformState &State) { | |||
auto *InvarCond = | |||
isInvariantCond() ? State.get(getCond(), VPIteration(0, 0)) : nullptr; | |||
|
|||
// FIXME: Do we have a scenario where cond is a scalar? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to add assertion here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something, but can't this happen when the condition is a loop invariant value? If so, this should have a. test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added assertion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you. I tried to construct a test case from C that would satisfy this, but it didn't sucess, so I added an assertion.
55f67be
to
8b6eb58
Compare
Any other questions? Can I merge it ? @fhahn @alexey-bataev |
8b6eb58
to
0053b2a
Compare
I am wondering if there's a better way to avoid duplicating all or most recipes for EVL. Would it be sufficient to create recipes to widen the appropriate VPIntrinsic directly at least for some cases? This would need allowing creating widened intrinsic recipes without underlying call instruction or function object (#110486). Then introducing vp.select (and others) can be added with a few lines like in #110489 |
I’ve also been seriously thinking about this recently. But my thinking is whether it’s really necessary to create an EVL version for each recipe, especially after noticing VLOpt #108640. I think maybe we only need to create EVL recipes for a small subset of recipes, such as those related to memory accesses (load, store, interleaved accesses), in-loop reduction... etc. IIUC, VLOpt should have the ability to prune the VL.
In conclusion, we only need to transforms the partial recipes into EVL recipes. As for which ones exactly, we can determine by whether the recipe has a mask operand to decide if an EVL recipe is necessary. What do you think? |
If possible, I think it is better to avoid creating EVL that is larger than required. The first reason is that the VLOpt pass is not very mature and may miss some cases. The original intention of the pass is to fix the case introduced by vector GEP which has no EVL based version. In that case, there was no way for the compiler to use the correct VL at the IR level which makes that pass necessary. But if the compiler can avoid introducing larger than needed EVL, that is preferred, since it will create less work for the VLOpt pass, which will improve compiler times. |
+1 |
If triple count is not a constant, can it be directly introduced into EVL? |
No description provided.