[PATCH] D99569: [LoopVectorize] Fix bug where predicated loads/stores were dropped

[Joe Ellis via Phabricator via llvm-commits]

joechrisellis added a comment.

Hi @fhahn -- thanks for the review!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1515
+  bool isPredicatedInst(Instruction *I,
+                        ElementCount VF = ElementCount::getFixed(1)) {
     if (!blockNeedsPredication(I->getParent()))
----------------
fhahn wrote:
> I'm not sure about the default. Why does it make sense to default to VF = 1?  Can all users of the function instead pass the right VF?
The reason I added this optional parameter is because `isPredicatedInst` is used in this context:

```
bool IsPredicated = LoopVectorizationPlanner::getDecisionAndClampRange(
    [&](ElementCount VF) { return CM.isPredicatedInst(I, VF); }, Range);
```

I wanted to make sure that the VF (which is used to make decisions on how to clamp the range) is made available to `isScalarWithPredication` in the fallback case for this particular call, but keep the same signature for calls elsewhere. Given what I think the function is supposed to do, I can see why it might not make sense to pass a VF parameter for the general case, but here I _think_ we need it. Other references to `isPredicatedInst` do not supply a VF parameter, so in the fallback cases, they will default to VF = 1 when we reach the call for `isScalarWithPredication`. This is just pushing the optional parameter up the stack so that it can be used for `isPredicatedInst` too.

If there's a better thing to do here I'm happy to make changes. 😄 


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:21
+;
+;   1   void foo(int *restrict data1, int *restrict data2)
+;   2   {
----------------
fhahn wrote:
> this seems already out of sync with the IR below.... Shouldn't the `restrict` be translated to `noalias` in the function definition below? FWIW I think it would be more helpful if you'd add a comment to the test saying this checks that the store in the `if.then` block gets properly predicated if VF = 1 or something like that, as the IR is already quite compact :)
Good spot. I'll remove the `restrict` in the C code example since it isn't strictly necessary here anyways.


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:45
+;
+; DBG: LV: Scalarizing and predicating:  store i32 %1, i32* %arrayidx, align 4
+
----------------
fhahn wrote:
> Is this check crucial? Seems like the IR checks already check this and we could remove it and execute it even with asserts.
Fair enough, done!


================
Comment at: llvm/test/Transforms/LoopVectorize/AArch64/scalarize-store-with-predication.ll:56
+  %1 = load i32, i32* %arrayidx2, align 4
+  %cmp = icmp sgt i32 %0, %1
+  br i1 %cmp, label %if.then, label %if.end
----------------
fhahn wrote:
> do we need 2 loads here or could one be removed and compare to a constant?
I would like to keep this IR as-is for consistency with the C code at the top of the test. 🙂 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99569/new/

https://reviews.llvm.org/D99569