[PATCH] D68128: [InstCombine] Fold PHIs with equal incoming pointers

[Daniil Suchkov via Phabricator via llvm-commits]

DaniilSuchkov added a comment.

In D68128#1689376 <https://reviews.llvm.org/D68128#1689376>, @lebedev.ri wrote:

> I see.
>  So we want to hoist identical gep+bitcast from 2 BB's into their closest dominating BB, more or less.
>  Do we have standalone hoisting pass?


As stated in the "Summary", the main point is not to just hoist expressions, but to canonicalize them in order to open opportunities for further optimizations, so hoisting is more of a side-effect here (I chose it because sinking doesn't work). InstCombine sounds like the right place for canonicalization of specific simple patterns of instructions. GVN performs transformations of this kind but it takes number of uses into account (while for canonicalization it shouldn't matter) and also won't handle cases when the sequences of instructions producing equal pointers are different.

InstCombine already handles such patterns (is sinks instructions one by one) but with some serious limitations: only identical sequences of instructions are allowed and only for cases where PHI is the only user. If the latter restriction is lifted, the resulting code is then "optimized" back by GVN.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1157-1168
+  Instruction *InsertPt = nullptr;
+  if (auto *BaseInst = dyn_cast<Instruction>(Base)) {
+    if (isa<PHINode>(BaseInst)) {
+      BasicBlock *InsertBB = BaseInst->getParent();
+      BasicBlock::iterator InsertPtIter = InsertBB->getFirstInsertionPt();
+      // Make sure the insertion point exists.
+      if (InsertPtIter != InsertBB->end())
----------------
lebedev.ri wrote:
> I'm not sure if this is correct.
> I'd expect that we actually need to check to where we can move these geps.
> 
Why do you think it's incorrect? Given the offset is constant, this code just looks for an insertion point right after base pointer definition and bails out in case when such point doesn't exist.

What restrictions did I miss?


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

https://reviews.llvm.org/D68128