[PATCH] D143720: [InstCombine] extend "simplifyUsingControlFlow" supporting zext/sext/trunc for different sizes

Tue Mar 7 06:22:12 PST 2023

inclyc added inline comments.

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1307
   Value *Cond;
-  SmallDenseMap<ConstantInt *, BasicBlock *, 8> SuccForValue;
+  SmallDenseMap<ConstantInt *, BasicBlock *, 8> SuccValue, SuccValueTrunc,
+      SuccValueZExt, SuccValueSExt;
----------------
khei4 wrote:
> IMHO, duplicating `SuccForValue` map for each casts seems a little redundant. I like collecting incoming phi values in the front and checking correspondences to unify those maps. Then we can also early return if the values don't correspond.
Thanks for the feedback! 

>  I like collecting incoming phi values in the front and checking correspondences to unify those maps.

Can you elaborate your solution? Here are my worries:

Here we are going to find a operation that maps branch condition in to corresponding phi values. Not an operation that maps phi values back to branch condition.

I think I have to left "sext"/"zext"ed values here because we must know the result of the branch condition after "s/zext".  That's different from truncating from PHI values. i.e. s/zext information is not recoverable. Truncating phi values to branch condition does not imply we can z/sext the branch condition back to "phi values".

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1379-1381
+  // This Phi is actually opposite to branching condition of IDom after
+  // zext/sext/trunc. We invert the condition that will potentially open up some
+  // opportunities for sinking.
----------------
khei4 wrote:
> Inverting can't be determined on these lines. If you do so, these comments look obsolete, and better to be modified
Nice catch! I'd like to fixup this after all "functional change" reviews.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143720