This is an archive of the discontinued LLVM Phabricator instance.

[VPlan] Manage compare predicates in VPRecipeWithIRFlags.
ClosedPublic

Authored by fhahn on Aug 28 2023, 7:54 AM.

Details

Summary

Extend VPRecipeWithIRFlags to also manage predicates for compares. This
allows removing the custom ICmpULE opcode from VPInstruction which was a
workaround for missing proper predicate handling.

This simplifies the code a bit while also allowing compares with any
predicates. It also fixes a case where the compare predixcate wasn't
printed properly for VPReplicateRecipes.

Discussed/split off from D150398.

Diff Detail

Event Timeline

fhahn created this revision.Aug 28 2023, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2023, 7:54 AM
fhahn requested review of this revision.Aug 28 2023, 7:54 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 28 2023, 7:54 AM
Ayal added a comment.Aug 30 2023, 6:18 AM

Thanks for following up on this! Adding various nits.

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
122–123

Add documentation? TODO add createFCmp() when needed?

Perhaps better place below, close to createNot/And/Or/Select, after the more general createNaryOp().
Have it returned createInstruction() like them, with a new createInstruction() that calls the new VPInstruction constructor?

BTW, the createNaryOp() with Inst seems a bit odd?

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

nit: place after FPMathOp, to be consistent with the union order below?

860

nit: keep AllFlags last? An unsigned char still suffices, right?

877

nit: place below after handling FPMathOperator, for consistency.

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

Is FCmp case dead, or checked by any test?

OTOH, would be ideal to generalize by checking if Instruction may have side effects for all opcodes up to Instruction::OtherOpsEnd, but w/o having an Instruction instance...

629

nit: place below for consistency.

llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll
909

nice catch!

Ayal added inline comments.Aug 30 2023, 6:49 AM
llvm/lib/Transforms/Vectorize/VPlan.h
1075–1076

Also have clone() replicate the IR flags? (Independent of this patch, calls for testing?)

tschuett added inline comments.Aug 30 2023, 7:02 AM
llvm/lib/Transforms/Vectorize/VPlan.h
854–855

No std::variant?

fhahn updated this revision to Diff 554738.Aug 30 2023, 8:40 AM
fhahn marked 8 inline comments as done.

Address latest comments, thanks!

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
122–123

Added TODO and documentation and moved.

Using createInstruction would require adding a new variant of that function I think. Maybe it would be better to just add a helper to insert the created instruction.

BTW, the createNaryOp() with Inst seems a bit odd?

Used for VPlan-native path I think. Should be able to remove it once we add an in-place IR recipe.

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

Moved, thanks!

854–855

Hmm, can be changed in the future. It looks like there are very few uses of std::variant in the llvm subdirectory so it doesn't seem used a lot.

860

No, updated the unsigned thanks! (Although all predicates should fit into a char...)

877

Updated to have Cmp at top of the enum and union.

1075–1076

Seemed unused, will remove separately.

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

Dead code for now, removed!

OTOH, would be ideal to generalize by checking if Instruction may have side effects for all opcodes up to Instruction::OtherOpsEnd, but w/o having an Instruction instance...

Agreed, that would require refactoring the corresponding implementation in Instruction to just work on opcodes.

629

Reordered code to have Cmp at front everywhere.

Ayal added a comment.Aug 31 2023, 12:07 AM

Reordered code to have Cmp at front everywhere.

Fine, let's have it at front everywhere.

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
49

nit: tryInsertInstruction?

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

nit: place this first as well

999

nit: let Cmp appear first consistently

1074

nit: ditto

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

Perhaps worth leaving behind a TODO.
Perhaps native may find this useful already?

267

nit: ditto

283

nit: FCmp is currently dead?

fhahn marked 6 inline comments as done.Sep 1 2023, 2:30 AM
fhahn added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
49

Adjusted, thanks!

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

Moved, thanks!

999

Moved, thanks!

1074

Moved, thanks!

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

Moved, thanks!

283

dropped, thanks!

fhahn updated this revision to Diff 555314.Sep 1 2023, 2:31 AM
fhahn marked 6 inline comments as done.

Address comments, thanks!

Ayal accepted this revision.Sep 1 2023, 12:41 PM

LGTM, adding last couple of nits. thanks!

llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
163

nit: assert Pred is an Integer compare predicate.

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

nit: assert Opcode is an ICmp; introduce support for FCmp later when needed.

This revision is now accepted and ready to land.Sep 1 2023, 12:41 PM
This revision was automatically updated to reflect the committed changes.
fhahn marked 2 inline comments as done.
fhahn added inline comments.Sep 2 2023, 1:46 PM
llvm/lib/Transforms/Vectorize/LoopVectorizationPlanner.h
163

Added in the committed version, thanks!

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

Removed in the committed version, thanks!