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

[Allen zhong via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4285
+    // Avoid some regression case.
+    if (isa<SelectInst>(I) || isa<CmpInst>(I) || isa<LoadInst>(I))
+      return V;
----------------
goldstein.w.n wrote:
> Could you explain a bit more about why this is necessary for avoiding a regression?
Thanks for your attrention.
1) I think it is not right to try this because **isa<SelectInst>(I)** have a **select operand**, such as case ashr_out_of_range_1 in file Transforms/InstCombine/shift.ll.
```
  %1 = icmp eq i177 %L, -1,  %L = load i177, ptr %A, align 4,i177 -1
  %B = select i1 %1, i177 0, i177 %L,  %L = load i177, ptr %A, align 4,i177 -1
   
  It is not allowed to deduce the value of %B  is i177 0 when we recursive try the selection operand %1 with i1 true.
  we usual try to replace the compare operands, refer to simplifySelectWithICmpEq.
```
2) For  **isa<LoadInst>**, There will be some crash when we recursive try the pointer operand, which usual is a GetElementPtrInst, so its type is not a vector type, and bring in crash in the above assert in branch **if (Op->getType()->isVectorTy())**.  On the other hand, the value of load is not confirmed, and it is difficult to further optimize.

3) skip **isa<CmpInst>(I)** is not necessary, so I'll revert it.


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

https://reviews.llvm.org/D153698