[PATCH] D153698: [InstCombine] canonicalize multi xor as cmp+select

[Allen zhong via Phabricator via llvm-commits]

Allen marked 5 inline comments as done.
Allen added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4256
   // We cannot replace a constant, and shouldn't even try.
-  if (isa<Constant>(Op))
-    return nullptr;
-
-  auto *I = dyn_cast<Instruction>(V);
-  if (!I || !is_contained(I->operands(), Op))
+  if (isa<Constant>(Op) || !isa<Instruction>(V))
     return nullptr;
----------------
nikic wrote:
> As we're now doing recursive calls, you need to guard against `MaxRecurse == 0` here.
Done, thanks


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4276
   transform(I->operands(), NewOps.begin(),
             [&](Value *V) { return V == Op ? RepOp : V; });
 
----------------
nikic wrote:
> Allen wrote:
> > nikic wrote:
> > > This isn't what I had in mind. Why can't we do the recursive call in here?
> > I think there is conflict on the solution.
> > we need check **!is_contained(I->operands(), Op)** then entry the  recursive call , while in the transform, we can't get all the **operands of I**
> You need to keep track whether an operand has been replaced or not. Previously this was just done by is_contained, but now you would have to check the return value of the recursive simplifyWithOpReplaced. If there is no replacement, the following code can be skipped.
Thanks, apply your comment, add changed to track that.


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

https://reviews.llvm.org/D153698