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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1515
+  bool isPredicatedInst(Instruction *I,
+                        ElementCount VF = ElementCount::getFixed(1)) {
     if (!blockNeedsPredication(I->getParent()))
----------------
joechrisellis wrote:
> 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. 😄 
> his is just pushing the optional parameter up the stack so that it can be used for isPredicatedInst too.

That's fair enough, but my point was that there are only 2 uses of the function I think, so the default parameter is not really that useful and I think it would be better to just explicitly pass the VF in at both call sites. Also, the default of VF(1) is surprising (but I realize it is used elsewhere) and it's probably better to force the caller to think about what VF to pass in.


================
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
----------------
joechrisellis wrote:
> 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. 🙂 
> I would like to keep this IR as-is for consistency with the C code at the top of the test. 🙂

Personally I think that the C example code should not stand in the way of having a simpler IR test. If you really want to keep the C code, can't you simplify the C as well? (With the even simpler IR I think the C code adds more noise than value it adds).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99569