[PATCH] D51313: [LV] Fix code gen for conditionally executed uniform loads
Hideki Saito via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 6 12:23:38 PDT 2018
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
More information about the llvm-commits
mailing list