This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Replace remaining use of needsScalarIV.
ClosedPublic

Authored by fhahn on Apr 11 2022, 1:15 PM.

Details

Summary

All information is already available in VPlan. Note that there are some
test changes, because we now can correctly look through instructions
like truncates to analyze the actual users.

Diff Detail

Event Timeline

fhahn created this revision.Apr 11 2022, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 1:15 PM
fhahn requested review of this revision.Apr 11 2022, 1:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 1:15 PM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal added a comment.Apr 13 2022, 9:10 AM

Note that there are some test changes, because we now can correctly look through instructions like truncates to analyze the actual users.

this should be faster(?) ... and in some cases help clarify that a loop was actually not "vectorized" - when vector loop body is free of any vector instructions.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8183–8184

NeedsScalarIV is now dead?

8184

When/can NeedsScalarIVOnly also be discarded?

llvm/lib/Transforms/Vectorize/VPlan.h
2977

nit: name is a bit cryptic, but a better alternative doesn't come to mind.
Suffice to have a lambda next to the single user?

llvm/test/Transforms/LoopVectorize/induction-multiple-uses-in-same-instruction.ll
25

hmm, is this still considered to be vectorized by 2 and interleaved by 1, rather that the converse, contrary to the prescribed "-force-vector-width=2 -force-vector-interleave=1"

llvm/test/Transforms/LoopVectorize/single-value-blend-phis.ll
287

Is it important to add above CHECKs for the scalar loop?

332

ditto

fhahn updated this revision to Diff 422616.Apr 13 2022, 12:52 PM
fhahn marked an inline comment as done.

Address latest comments.

Note that there are some test changes, because we now can correctly look through instructions like truncates to analyze the actual users.

this should be faster(?) ... and in some cases help clarify that a loop was actually not "vectorized" - when vector loop body is free of any vector instructions.

Agreed!

fhahn marked 5 inline comments as done.Apr 13 2022, 12:56 PM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8183–8184

Removed.

8184

I think the main remaining difference is the handling of vector compares for predication, where we would now not generate vector phis. I'll take a look.

llvm/lib/Transforms/Vectorize/VPlan.h
2977

Inlined, thanks!

llvm/test/Transforms/LoopVectorize/induction-multiple-uses-in-same-instruction.ll
25

Yeah, now it's a bit clearer, the main intention of the test is to check a case where %iv has multiple uses in the same instruction.

llvm/test/Transforms/LoopVectorize/single-value-blend-phis.ll
287

Removed!

332

Removed.

Ayal accepted this revision.Apr 13 2022, 1:27 PM

Looks good to me, curious if it may lead to any regression...
Adding minor nits.

llvm/lib/Transforms/Vectorize/VPlanTransforms.cpp
378

nit (irrespective of this patch): would be good to have a more descriptive name, e.g., optimizeScalarInductions().

382

nit: a comment should explain that if no user of IV uses scalars, there's no need for an optimized scalar induction.
nit: may be clearer to use none_of() instead of all_of() with !

395–396

nit: "Step" >> "Steps"

This revision is now accepted and ready to land.Apr 13 2022, 1:27 PM
fhahn updated this revision to Diff 435450.Jun 9 2022, 1:32 AM
fhahn marked 5 inline comments as done.

Rebase, I will land this shortly.

This revision was landed with ongoing or failed builds.Jun 9 2022, 4:06 AM
This revision was automatically updated to reflect the committed changes.