This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Tail-merging all blocks with `ret` terminator
ClosedPublic

Authored by lebedev.ri on Jun 19 2021, 11:57 AM.

Details

Summary

Based ontop of D104598, which is a NFCI-ish refactoring.
Here, a restriction, that only empty blocks can be merged, is lifted.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jun 19 2021, 11:57 AM
lebedev.ri requested review of this revision.Jun 19 2021, 11:57 AM
lebedev.ri edited the summary of this revision. (Show Details)

Split off mostly NFC refactor into a separate change.

Autogenerate more, but still not all, affected codegen tests.

rnk added a comment.Jun 22 2021, 3:28 PM

I think this is reasonable: later cfg simplifications will tail merge the preceding instructions and delete dbg.values appropriately. I didn't hit "accept" since the tests don't all pass yet.

llvm/test/CodeGen/AArch64/GlobalISel/irtranslator-condbr-lower-tree.ll
37 โ†—(On Diff #353229)

This transform has the potential to make it harder to recognize some tail calls during codegen, but it's clear from this test that codegen is already smart enough to handle this case. I don't think any change is needed.

llvm/test/CodeGen/ARM/smml.ll
168 โ†—(On Diff #353229)

I can't understand why these checks disappeared. Is this a bug in the check generation script?

llvm/test/Transforms/PruneEH/ipo-nounwind.ll
33

Seems like another update bug.

I think this is reasonable: later cfg simplifications will tail merge the preceding instructions and delete dbg.values appropriately. I didn't hit "accept" since the tests don't all pass yet.

Thank you for taking a look!
This is a much warmer reaction than i was expecting.

Autogenerated a few more tests. Still a few more to go.

lebedev.ri edited the summary of this revision. (Show Details)

@rnk done, all tests updated.

ormris removed a subscriber: ormris.Jun 23 2021, 9:32 AM

Some of the test changes here look really ropy, but the performance benchmarks I tested was entirely flat, and codesize neither improved nor regressed by any significant amount.

There are some tests in here, notably the ifcvt ones, that no longer seem to be testing what they were originally intending to test.

llvm/test/CodeGen/AArch64/arm64-instruction-mix-remarks.ll
42 โ†—(On Diff #353962)

I don't know this test, but what happened to this store?

llvm/test/CodeGen/ARM/smml.ll
67 โ†—(On Diff #353962)

These look like they can share CHECK-V6V7?

84 โ†—(On Diff #353962)

Is this left over?

rnk added a comment.Jun 23 2021, 1:49 PM

I'm pretty sure Clang generates a single return block, so this change alone probably won't affect any C/C++ benchmarks. Most of the affected tests are manually written IR, where LLVM developers tend to use multiple ret terminators for simplicity.

I believe Clang also uses a single resume block, but I could be wrong.

lebedev.ri marked 3 inline comments as done.

@dmgreen thank you for taking a look! Tidyed-up the two tests you pointed out, hopefully it's better now.

@rnk do tell if there was something else i was supposed to do here..

I'm pretty sure Clang generates a single return block, so this change alone probably won't affect any C/C++ benchmarks. Most of the affected tests are manually written IR, where LLVM developers tend to use multiple ret terminators for simplicity.

I believe Clang also uses a single resume block, but I could be wrong.

Ah. That would explain the performance being flat. Codesize is different enough to suggest something is changing, but it's not enough of a difference to worry about.

rnk accepted this revision.Jun 23 2021, 8:46 PM

lgtm

This revision is now accepted and ready to land.Jun 23 2021, 8:46 PM
fhahn added inline comments.Jun 24 2021, 1:38 AM
llvm/test/CodeGen/AArch64/swifterror.ll
429 โ†—(On Diff #354075)

I noticed you auto-generated the tests for this file and the GlobalISel version. It looks like those tests intentionally only checked the relevant parts related to swifterror, but now the checks are much more verbose (and much more prone to needing updating, because they check lots of 'uninteresting things).

Given that only a few lines out of ~2000 needed updating, I am wondering if it would be better to keep the hand-written checks and just update the failing line? Going forward, I think the auto-generated ones will need updating much more frequently now. Granted it is easy to update them, but it still creates noise in diffs and in theory would require careful checking.

fhahn added inline comments.Jun 24 2021, 1:42 AM
llvm/test/CodeGen/AArch64/swifterror.ll
429 โ†—(On Diff #354075)

(The original checks also had comments interleaved to explain what is being checked. It looks like they are now all before the auto-generated check lines)

lebedev.ri marked 2 inline comments as done.

@rnk thank you for the review!

Address nit from @fhahn.

This revision was landed with ongoing or failed builds.Jun 24 2021, 3:20 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jun 25 2021, 2:28 AM

Great, thanks!

jgorbe added a subscriber: jgorbe.Jul 20 2021, 5:20 PM