[PATCH] D51313: [LV] Fix code gen for conditionally executed uniform loads

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:6826
     if (isa<LoadInst>(I) || isa<StoreInst>(I)) {
-      assert(CM.getWideningDecision(I, VF) ==
-                 LoopVectorizationCostModel::CM_Scalarize &&
+      assert((CM.getWideningDecision(I, VF) !=
+                  LoopVectorizationCostModel::CM_Widen &&
----------------
hsaito wrote:
> anna wrote:
> > hsaito wrote:
> > > Same as Line 4404.
> > I believe it does not apply here. We specifically *need* to check for widening decisions. At this point, the decision can be CM_GATHERSCATTER or a widening decision (if we hadn't called `tryToWidenMemory` and fail this assert). We will never get CM_Scalarize here because of the check at line 6747.
> > 
> > So, I think this assert is pretty clear in that aspect.
> In non-NativeVPlan mode, tryToWidenMemory() is the only place that creates VPWidenMemoryRecipe(), which handles Widen, Widen_Reverse, and GatherScatter. If we intend to emit vector loads/stores or gathers/scatters, that needs to be caught there.
> 
> Line 6747 filters out only the predicated scalars. So, unmasked scalar loads/stores would hit here.
> 
> So, in my opinion, if CM_GatherScatter hits here, that means "cost model wants to use gather/scatter but we'll serialize". That is an error condition, isn't it?
> Line 6747 filters out only the predicated scalars. So, unmasked scalar loads/stores would hit here.
agreed. that's right. So we can hit CM_Scalarize here.

> So, in my opinion, if CM_GatherScatter hits here, that means "cost model wants to use gather/scatter but we'll serialize". That is an error condition, isn't it?
Actually, that doesn't agree with the error message though: "Memory widening decisions should have been taken care by  now".

Also, even if we hit CM_GatherScatter here, we will not widen because `willWiden` returns false. 

Basically, this function `tryToWiden` does not handle loads and stores of any kind: by this point, we should finish `CM_Widen, Widen_reverse, GatherScatter and CM_Interleave`.

So, I think you mean that we should leave the assert as-is because only non-predicated loads and stores of CM_Scalarize are allowed to reach here? 
Thanks for clearing this.

Also, the error message is really unclear in that aspect. It's not that widening isn't allowed here - none of the other widening decisions are allowed. It should state something like "only  unmasked loads and stores allowed here"


Repository:
  rL LLVM

https://reviews.llvm.org/D51313