[PATCH] D78430: [InstSimplify] fold and/or of compares with equality to min/max constant

[Sanjay Patel via Phabricator via llvm-commits]

spatel planned changes to this revision.
spatel marked 3 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:1486
-  if (UnsignedPred == ICmpInst::ICMP_ULT && EqPred == ICmpInst::ICMP_NE)
-    return IsAnd ? UnsignedICmp : ZeroICmp;
-
----------------
aqjune wrote:
> Is simplification on pointer comparisons still alive after this is removed? 
Good catch! No, we would lose nullptr simplifications. But there were no regression tests for those cases in InstSimplify or any other other pass. Let me add some. Given that, we can't just remove the specializations for compare with 'null'.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:1488-1492
   // X <= Y && Y != 0  -->  X <= Y  iff X != 0
   // X <= Y || Y != 0  -->  Y != 0  iff X != 0
   if (UnsignedPred == ICmpInst::ICMP_ULE && EqPred == ICmpInst::ICMP_NE &&
       isKnownNonZero(X, Q.DL, /*Depth=*/0, Q.AC, Q.CxtI, Q.DT))
     return IsAnd ? UnsignedICmp : ZeroICmp;
----------------
lebedev.ri wrote:
> Any handling plan for these neighbor patterns?
I didn't look closely, but I assumed we can't generalize these because of the isKnownNonZero() dependency. We'd have to look at the regression tests to see if there's some other means of value tracking to generalize the expected cases.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:1675
+static Value *simplifyAndOrOfICmpsWithLimitConst(ICmpInst *Cmp0, ICmpInst *Cmp1,
+                                                 bool IsAnd) {
+  // Canonicalize an equality compare as Cmp0.
----------------
lebedev.ri wrote:
> You literally always check `!IsAnd`.
> Should this be `IsOr` instead?
I thought someone would call that out. :)
My mind defaulted to the 'and' versions of the patterns when visualizing the logic, so that's how the code ended up like this. If we are comfortable inverting the pattern matching to the 'or' versions, then we can invert the first check at least to the positive 'IsAnd'. 
Another option would duplicate all of the code predicated by the 'IsAnd' check.
The alternative - make this 'IsOr' - would mean this function stands different/alone compared to all of the related compare simplifications. That didn't seem like a worthy option to me.
Let me know which, if any, form you'd prefer.


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

https://reviews.llvm.org/D78430