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

[Kohei Asano via Phabricator via llvm-commits]

khei4 added a comment.

BTW, the original issues seem to expect arithmetic negation, not bit negation, it might be good to test it also. (I'm new to around this,  so those comments are just my opinions.



================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1307
   Value *Cond;
-  SmallDenseMap<ConstantInt *, BasicBlock *, 8> SuccForValue;
+  SmallDenseMap<ConstantInt *, BasicBlock *, 8> SuccValue, SuccValueTrunc,
+      SuccValueZExt, SuccValueSExt;
----------------
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.


================
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.
----------------
Inverting can't be determined on these lines. If you do so, these comments look obsolete, and better to be modified


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp:1385
     Self.Builder.SetInsertPoint(&*InsertPt);
-    return Self.Builder.CreateNot(Cond);
+    if (CondSize < PNSize) {
+      auto [ZExtOk, ZExtInvert] = CheckSuccValue(SuccValueZExt);
----------------
IMHO, this checking, which is done when creating maps, seems a bit redundant, it's better to hold conditions in some way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143720