This is an archive of the discontinued LLVM Phabricator instance.

[MachineCombiner] Lift same-bb restriction for reassociable ops.
ClosedPublic

Authored by fhahn on Jan 9 2023, 9:38 AM.

Details

Summary

This patch relaxes the restriction that both reassociate operands must
be in the same block as the root instruction.

The comment indicates that the reason for this restriction was that the
operands not in the same block won't have a depth in the trace.

I believe this is outdated; if the operand is in a different block, it
must dominate the current block (otherwise it would need to be phi),
which in turn means the operand's block must be included in the current
rance, and depths must be available.

There's a test case (no_reassociate_different_block) added in
70520e2f1c5fc4 which shows that we have accurate depths for operands
defined in other blocks.

This allows reassociation of code that computes the final reduction
value after vectorization, among other things.

Diff Detail

Event Timeline

fhahn created this revision.Jan 9 2023, 9:38 AM
fhahn requested review of this revision.Jan 9 2023, 9:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2023, 9:38 AM

This sounds OK to me. It looks like there are more tests that need updating.

fhahn updated this revision to Diff 488380.Jan 11 2023, 2:14 PM

Update tests missed in initial revision.

dmgreen accepted this revision.Jan 12 2023, 12:29 AM
dmgreen added a reviewer: craig.topper.

Thanks. LGTM, but perhaps wait a day or so in case these test changes look worse than I think they do.

This revision is now accepted and ready to land.Jan 12 2023, 12:29 AM
fhahn updated this revision to Diff 488988.Jan 13 2023, 6:47 AM

Thanks! Rebased, planning on landing this soon.

This revision was landed with ongoing or failed builds.Jan 13 2023, 7:33 AM
This revision was automatically updated to reflect the committed changes.

Hi, I am seeing an 8% degradation in SPEC 2006 Soplex after this patch. Created an issue here: https://github.com/llvm/llvm-project/issues/62100