[PATCH] D148986: [InstSimplify] with logical ops: (X | Y) ? 0 : X --> 0

[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:4597
+                                            Q, MaxRecurse))
+      return V;
   }
----------------
nikic wrote:
> Not quite the code structure I had in mind. Rather than handling or in simplifySelectWithICmpEq, I'd expect something like this in here:
> 
> ```
> if (match(CmpLHS, m_Or(m_Value(X), m_Value(Y))) && match(CmpRHS, m_Zero())) {
>   // X | Y == 0 implies X == 0 and Y == 0.
>   if (Value *V = simplifySelectWithICmpEq(X, CmpRHS, TrueVal, FalseVal, Q,
>                                           MaxRecurse))
>       return V;
>   if (Value *V = simplifySelectWithICmpEq(Y, CmpRHS, TrueVal, FalseVal, Q,
>                                           MaxRecurse))
>       return V;
> }
> ```
> 
> That is, we handle the straightforward equality, and then we try to handle the implied equalities from the icmp or.
Thanks for detail suggestion, apply your comment.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4733
+      match(TrueVal, m_ZeroInt()) && Pred == ICmpInst::ICMP_NE)
+    return TrueVal;
+
----------------
RKSimon wrote:
> Also - is this safe when matching zero vectors with undef elements?
Thanks, added with select_icmp_or_eq_vec


================
Comment at: llvm/test/Transforms/InstSimplify/select-cmp-or.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt < %s -passes=instsimplify,instcombine -S | FileCheck %s
+
----------------
nikic wrote:
> Why does this also run instcombine?
Delete it now,thanks.
I added this because the scenario of the case **select_icmp_or_ne** cannot be directly addressed, while I hope this scenario can also be monitored and verified.


================
Comment at: llvm/test/Transforms/InstSimplify/select.ll:1408
+
+define i32 @select_icmp_or(i32 noundef %a, i32 noundef %b) {
+; CHECK-LABEL: @select_icmp_or(
----------------
RKSimon wrote:
> is noundef necessary?
I deleted the noundef, thanks


================
Comment at: llvm/test/Transforms/InstSimplify/select.ll:1416
+  ret i32 %cond
+}
----------------
RKSimon wrote:
> commutation test?
Added with case select_icmp_or_eq_commuted


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

https://reviews.llvm.org/D148986