Page MenuHomePhabricator

[VPlan] Move remove dead recipes before merging regions.
ClosedPublic

Authored by fhahn on Jun 29 2022, 8:49 AM.

Details

Summary

This can enable additional region merging, while not losing
opportunities as region merging does not produce dead recipes.

Diff Detail

Event Timeline

fhahn created this revision.Jun 29 2022, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 8:49 AM
fhahn requested review of this revision.Jun 29 2022, 8:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2022, 8:49 AM
Herald added a subscriber: vkmr. · View Herald Transcript
Ayal accepted this revision.Jun 30 2022, 1:33 PM

This minimal improvement is fine, thanks! Adding some optional nits:

Would "[VPlan] Move remove dead recipes before merging regions." be a clearer title?

This can enable additional region merging.

while OTOH not losing opportunities as region merging does not produce dead recipes?

Note that it may be useful for VPlan2VPlan transforms to return an indication if they made any changes.

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

Should removeDeadRecipes also run after mergeReplicateRegions, to clean-up such phi recipes it leaves behind?

1143

Hmm, this dead_gep may indeed prevent regions to merge, but is it visible in VPlan?

This revision is now accepted and ready to land.Jun 30 2022, 1:33 PM
fhahn retitled this revision from [VPlan] Remove dead recipes before merging regions. to [VPlan] Move remove dead recipes before merging regions..Jul 6 2022, 8:35 PM
fhahn edited the summary of this revision. (Show Details)
fhahn marked 2 inline comments as done.Jul 6 2022, 8:37 PM
fhahn added inline comments.
llvm/test/Transforms/LoopVectorize/vplan-sink-scalars-and-merge.ll
1106

Yes, those should also be removed, what is missing is handling in mayHaveSideeffects, which I am also planning on adding as follow-up.

1143

Yes it should be, but we only print the plans once, after all transforms have been applied, so it is not shown in the dump

This revision was landed with ongoing or failed builds.Jul 6 2022, 8:39 PM
This revision was automatically updated to reflect the committed changes.
fhahn marked 2 inline comments as done.