This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Model wrap flags directly, remove *NUW opcodes (NFC)
ClosedPublic

Authored by fhahn on Aug 5 2023, 8:46 AM.

Details

Summary

Model wrap flags directly using VPRecipeWithIRFlags and clean up the
duplicated *NUW opcodes.

Depends on D157144.

Diff Detail

Event Timeline

fhahn created this revision.Aug 5 2023, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2023, 8:46 AM
fhahn requested review of this revision.Aug 5 2023, 8:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2023, 8:46 AM
Ayal added inline comments.Aug 6 2023, 11:11 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8780–8782

nit (independent of this patch): comment above needs fixing.

llvm/lib/Transforms/Vectorize/VPlan.cpp
764–766

nit: can fold into a single

return isa<VPScalarIVStepsRecipe>(U) || isa<VPDerivedIVRecipe>(U)) 
           || cast<VPInstruction>(U)->getCode() == VPInstruction::CanonicalIVIncrement;
768
llvm/lib/Transforms/Vectorize/VPlan.h
860

Could the wrap flags be set instead by a (designated) constructor?

1043

Have a designated constructor that takes wrap flags as parameters and sets them?

fhahn updated this revision to Diff 547725.Aug 7 2023, 5:03 AM
fhahn marked 4 inline comments as done.

Address latest comments, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
8780–8782

Will do separately.

llvm/lib/Transforms/Vectorize/VPlan.cpp
764–766

Will do separately.

768

Adjusted, thanks

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

Updated! this needed exposing WrapFlagsTy to avoid ambiguous constructor calls (because the default DebugLoc and Twine args can be implicitly converted to bools)

Ayal accepted this revision.Aug 7 2023, 3:28 PM

Looks good to me! Adding a couple of final nits.

llvm/lib/Transforms/Vectorize/VPlan.cpp
764–766

Sure, thanks!

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
238

nit: can be provided inline.

309–310

nit: can replace false with hasNoSignedWrap(), here and below.

Whether this VPInstruction un/sign wraps better be decided upon its construction rather than execution.
Plus it's clearer.

This revision is now accepted and ready to land.Aug 7 2023, 3:28 PM
fhahn updated this revision to Diff 548109.Aug 8 2023, 1:52 AM

Address nits, thanks! Also rebased so this can be landed on top of current main.

This revision was landed with ongoing or failed builds.Aug 8 2023, 4:12 AM
This revision was automatically updated to reflect the committed changes.
fhahn marked 2 inline comments as done.
Ayal added a comment.Aug 8 2023, 4:28 AM

A couple of post-commit nits.

llvm/lib/Transforms/Vectorize/VPlan.h
972
llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
323
fhahn marked 3 inline comments as done.Aug 8 2023, 5:03 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/VPlan.cpp
764–766

Adjusted in e18a547ce21b, thanks!

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

Adjusted in b6d994de0f1f, thanks!

llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp
238

Moved, thanks!

309–310

Updated, thanks!

323

Adjusted in b6d994de0f1f, thanks!