This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Move Load/Store checks out of tryToWiden (NFC).
ClosedPublic

Authored by fhahn on Apr 12 2020, 7:59 AM.

Details

Summary

Handling LoadInst and StoreInst in tryToWiden seems a bit
counter-intuitive, as there is only an assertion for them and in no
case VPWidenRefipes are created for them.

I think it makes sense to move the assertion to handleReplication, where
the non-widened loads and store are handled.

Diff Detail

Event Timeline

fhahn created this revision.Apr 12 2020, 7:59 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2020, 7:59 AM
Ayal added a comment.Apr 12 2020, 3:55 PM

Thanks for this cleanup!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
6993

Another cleanup: remove PHI from this list and replace it with an assert(!isa<PHINode>(I)) at the outset, as PHIs are all handled by other, earlier recipes. This will further simplify willWiden() below, which can also absorb IsPredicated above: for non IsVectorizableOpcodes, handleReplication() will clamp the range according to IsPredicated anyhow.

(That will in turn lead to identical willWiden() across tryToWiden, tryToWidenSelect, and the inlined tryToWidenGEP ("Scalarize"); which suggests folding/further cleanup.)

7059

tryToWidenMemoryInstruction() is expected to have clamped the Range already; if not, Range should either be asserted to be clamped (w/o clamping) or it should be clamped (redundantly) but not under assert. Note that checking VF==1, Load or Store should be done alongside IsPredicated, outside getDecisionAndClampRange(), as they're independent of Range.

This assert, moved here from tryToWiden(), is practically the complementary of tryToWidenMemoryInstruction()'s willWiden(). Due to its complexity and redundancy, it should probably simply be removed.

fhahn updated this revision to Diff 256936.Apr 13 2020, 12:50 AM

Also remove PHI from vectorizable opcodes, remove assertion in handleReplication.

fhahn marked 2 inline comments as done.Apr 13 2020, 12:52 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7059

Yeah, initially I was hoping there would be a general way to assert that we only call handleReplication with instructions we expect to scalarize, but it seems like there is no general way to write such an assertion. The assertion I added is not that helpful in that regard, I've removed it as suggested.

Ayal accepted this revision.Apr 14 2020, 1:47 PM

Looks good to me, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7021

clang-format?

This revision is now accepted and ready to land.Apr 14 2020, 1:47 PM
fhahn marked 2 inline comments as done.Apr 15 2020, 2:19 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
7021

Fixed in committed version.

This revision was automatically updated to reflect the committed changes.
fhahn marked an inline comment as done.