This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Use isUniformAfterVec in VPReplicateRecipe::execute.
ClosedPublic

Authored by fhahn on Feb 21 2023, 7:35 AM.

Details

Summary

I was unable to find a case where this actually changes generated code,
but it enables the bug fix in D144434. It also brings codegen in line
with the handling of stores to uniform addresses in the cost model
(D134460).

Diff Detail

Event Timeline

fhahn created this revision.Feb 21 2023, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 7:35 AM
fhahn requested review of this revision.Feb 21 2023, 7:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 21 2023, 7:35 AM
Ayal added inline comments.Mar 8 2023, 8:37 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9572–9575

only needs a single only...

9575

This is a bit fragile considering that isUniformAfterVectorization() in general also represents lane-varying values whose first lane need only be used, as in vector stores - see isUniformDecision .

Ayal accepted this revision.May 11 2023, 8:49 AM

LGTM, with a couple of nits.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9572–9575
9575

This patch is fine - effectively considering Replicate recipe invariants as "uniform addresses" in addition to current isLiveIns (aka !hasDefiningRecipe).

Above comment (independent of this patch) refers to vputils which introduced "onlyFirstLaneUsed" to handle the "AfterVectorization" part, and its isUniformAfterVectorization() should be renamed isUniform(). In fact, it currently checks isInvariant(), i.e., same value across all loop iterations. This could be improved to "uniform" - same value across lanes (per part) as in D148841, and possibly also to same value across lanes*parts (per vectorized iteration) - but the latter refers to UF.

This revision is now accepted and ready to land.May 11 2023, 8:49 AM
Ayal added inline comments.May 11 2023, 4:12 PM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9575

Also, regarding isUniform - in addition to recipe-less LiveIns and uniform Replicate recipes, uniform values may also be provided by non-Replicate recipes placed in vec/pre-headers and by WidenGEP recipe (though broadcasted into a vector).

fhahn marked 5 inline comments as done.May 19 2023, 10:11 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
9572–9575

Fixed, thanks!

9575

Above comment (independent of this patch) refers to vputils which introduced "onlyFirstLaneUsed" to handle the "AfterVectorization" part, and its isUniformAfterVectorization() should be renamed isUniform(). In fact, it currently checks isInvariant(), i.e., same value across all loop iterations. This could be improved to "uniform" - same value across lanes (per part) as in D148841, and possibly also to same value across lanes*parts (per vectorized iteration) - but the latter refers to UF.

Let me do the renaming as follow-up. I think we shouldn't need any additional work to benefit from D148841 here; uniformity should be propagated to replicate recipes?

Also, regarding isUniform - in addition to recipe-less LiveIns and uniform Replicate recipes, uniform values may also be provided by non-Replicate recipes placed in vec/pre-headers and by WidenGEP recipe (though broadcasted into a vector).

This will be addressed by fixing the TODO in isDefinedOutsideVectorRegions, will do as follow-up once I have a suitable test case.

fhahn updated this revision to Diff 523836.May 19 2023, 10:12 AM
fhahn marked 2 inline comments as done.

Address comments and rebase, plan to land it soon.

This revision was landed with ongoing or failed builds.May 19 2023, 10:15 AM
This revision was automatically updated to reflect the committed changes.