[PATCH] D155940: [SimplifyCFG] Transform for merging the combination of phis in switch

[Hongyu Chen via Phabricator via llvm-commits]

XChy marked 5 inline comments as done.
XChy added a comment.

Thanks for your review! I understand your framing and it clear my mind. This framing is much general, not just for switch. And as you analyse, actually, a large part of the logic of this patch is copied from **TryToSimplifyUncondBranchFromEmptyBlock**, specified for switch. I restrict this fold in a narrow situation. Though this transformation is not directly related to switch, but I didn't apply such mergence to all BasicBlocks, since it's not always worthwhile as you say.

In switch, there are often empty case successors with only phis that can be merged into CommonDest, and such phis mergence

- Can be much more friendly to LookupTable optimization or something else.
- May prevent multiple calls to getCaseResults, I think.
- Bring better IR.
- Can serve as a canonicalization of switch.

But for other situation, I'm not sure if this mergence is always a good transform. I have concerns here:

- Too much inefficient mergeablity checks.
- Such mergence may not help with other transforms (Quite a few transforms are based on two branches, but not multiple branches)

Maybe it's possible to refactor **TryToSimplifyUncondBranchFromEmptyBlock** to implement the same thing, but it's too large and too hard for me.
I'll highly appreciate it if you have other suggestions or some knowledge about the stuff above.



================
Comment at: llvm/lib/Transforms/Utils/SimplifyCFG.cpp:6784
+  return true;
+}
+
----------------
nikic wrote:
> Unless I'm missing something, nothing here prevents CaseBB from dominating SuccBB, in which case there might be uses of phis from CaseBB outside of phis in SuccBB. Is the transform valid in that case?
CaseBB doesn't dominate SuccBB here. Because SuccBB is a case successor of switch, SuccBB has at least two predecessors (CaseBB, Switch Block). And what dominates SuccBB should be Switch Block or other predecessors, but not CaseBB.
`CaseBBs.contains(SuccBB)` guarantees it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155940/new/

https://reviews.llvm.org/D155940