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

[Hideki Saito via Phabricator via llvm-commits]

hsaito accepted this revision.
hsaito added a comment.
This revision is now accepted and ready to land.

LGTM



================
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 &&
----------------
anna wrote:
> 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"
>It should state something like "only unmasked loads and stores allowed here"

I agree. That would be clearer.


Repository:
  rL LLVM

https://reviews.llvm.org/D51313