This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by lebedev.ri on Jun 24 2021, 5:16 AM.

Details

Summary

Similar to what we already do for ret terminators.
As noted by @rnk, clang seems to already generate a single ret/resume,
so this isn't likely to cause widespread changes.

Diff Detail

Event Timeline

lebedev.ri created this revision.Jun 24 2021, 5:16 AM
lebedev.ri requested review of this revision.Jun 24 2021, 5:16 AM
rnk added inline comments.Jun 24 2021, 10:13 AM
llvm/test/Transforms/SimplifyCFG/tail-merge-resume.ll
33

I thought PHIs of structs were considered non-canonical, but it looks like LLVM creates them in this C++ example:

$ cat t.cpp
struct Cleanup {
  ~Cleanup();
};
void maythrow();
void resumeFromPhi() {
  Cleanup a;
  maythrow();
  Cleanup b;
  maythrow();
}

$ clang -S t.cpp  -o - -emit-llvm -O2 | grep phi
  %.pn = phi { i8*, i32 } [ %3, %lpad1 ], [ %2, %lpad ]

We should be OK then.

91–97

I think it's worth adding an extra block where the resume has a phi operand. That is the dominant pattern coming from clang, so it's important to show that simplifycfg can take a standalone resume block and merge it in.

lebedev.ri marked an inline comment as done.Jun 24 2021, 10:17 AM
lebedev.ri added inline comments.
llvm/test/Transforms/SimplifyCFG/tail-merge-resume.ll
33

They are canonical, and in fact i had to invent InstCombinerImpl::foldAggregateConstructionIntoAggregateReuse()
(and i still need to finish it) so that SimplifyCFGOpt::simplifyCommonResume() would actually work...
So yes, such PHI's are absolutely fine.

lebedev.ri marked an inline comment as done.

Revisit a test somewhat.

rnk accepted this revision.Jun 24 2021, 10:57 AM

lgtm, thanks

llvm/test/Transforms/SimplifyCFG/tail-merge-resume.ll
91–97

Phabricator is being weird, and this isn't highlighted as a diff, but I see that you added the semicommon block as suggested.

This revision is now accepted and ready to land.Jun 24 2021, 10:57 AM
lebedev.ri marked 2 inline comments as done.Jun 24 2021, 11:24 AM

@rnk thank you for the review!

llvm/test/Transforms/SimplifyCFG/tail-merge-resume.ll
91–97

That's because i already precommitted that, and rebased the diff over the precommitted change.

This revision was landed with ongoing or failed builds.Jun 24 2021, 11:26 AM
This revision was automatically updated to reflect the committed changes.
lebedev.ri marked an inline comment as done.