Add constant VPValues for versioned strides on VPlan construction. This
ensures the constant strides will be used directly.
Potential alternative to D147378.
Paths
| Differential D147783
[VPlan] Add stride->constant VPlan mapping at construction. ClosedPublic Authored by fhahn on Apr 7 2023, 5:02 AM.
Details
Diff Detail
Event TimelineComment Actions Found one more problem, the symbolic strides in vector.ph are not replaced, either, although it's negligible.
fhahn added a parent revision: D147892: [VPlan] Unify Value2VPValue and VPExternalDefs maps (NFCI)..Apr 9 2023, 1:03 PM fhahn added inline comments.
Comment Actions A couple points here.
On (2), be warned. I am not real comfortable with the PSE code quality. I'm reasonable sure we have bugs lurking that might be exposed if we started using it significantly more widely. Most of our usage in LV today is for generating checks and costing, adding it to the code generation path might cause some fallout. (And this is probably the biggest argument for not using PSE. I'm open to that argument, I'd just like to see it discussed explicitly.) fhahn marked 2 inline comments as done. Comment ActionsRebase and ping :)
Thanks for taking a look!
There is no need to restrict this to constant strides; if it is not constant (and not SCEVUnknown) then we would need to expand the SCEV, which can be done by introducing a VPExpandSCEVRecipe to take care of that.
We are moving towards modeling SCEV expansion explicitly in VPlan (this is already the case for expanding steps of inductions used in the plan). One the one hand this takes care of the issues we have with querying/expanding SCEVs while we already modified the CFG (and it is in an temporary invalid state). On the other hand this would also make it very easy to re-query PSE for all used SCEVs down the line. For this particular case, there's no SCEV expressions for the strides if it is used outside of inductions, so taking care of that using a mapping seems good first step to me. WDYT? fhahn added inline comments.
reames added inline comments.
This revision now requires changes to proceed.May 18 2023, 2:31 PM fhahn added inline comments. Comment Actions LGTM. I'm not super familiar with VPlan internals through, so you may want to wait for another reviewer. I'll defer to you on that.
fhahn marked 2 inline comments as done. Comment ActionsUpdate to RAUW and remove uneeded hasVPValueFor. fhahn added inline comments.
Comment Actions
Looks good to me too, ship it!
This revision was not accepted when it landed; it landed in state Needs Review.Jun 13 2023, 12:27 AM Closed by commit rGd209084720c4: [VPlan] Replace versioned stride with constant during VPlan opts. (authored by fhahn). · Explain Why This revision was automatically updated to reflect the committed changes. fhahn marked 3 inline comments as done. Comment Actions It introduced an assert in csky target, could you please reproduce it? @fhahn Comment Actions
Unfortunately I have no system with a csky libc. Could you share the IR before the crash? (e.g. by passing -mllvm -print-on-crash -mllvm -print-module-scope) Comment Actions
OK, thx. test.ll213 KBDownload ./bin/clang --target=csky-unknown-linux -DNDEBUG -mfloat-abi=hard -O2 -w -Werror=date-time -fcommon test.ll Comment Actions
Thanks, should be fixed by ea6ca9cb2b6
Revision Contents
Diff 523140 llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
llvm/lib/Transforms/Vectorize/VPlan.h
llvm/test/Transforms/LoopVectorize/ARM/mve-gather-scatter-tailpred.ll
llvm/test/Transforms/LoopVectorize/RISCV/strided-accesses.ll
llvm/test/Transforms/LoopVectorize/runtime-check-needed-but-empty.ll
|
Don't get why this cannot capture the cases in preheader?