[PATCH] D142345: [X86] Transform `(icmp eq/ne Abs(A), Pow2)` -> `(and/or (icmp eq/ne A,Pow2), (icmp eq/ne A,-Pow2))`

[Noah Goldstein via Phabricator via llvm-commits]

goldstein.w.n added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:53411
+
+      // With C as a power of 2 and C != 0 and C != INT_MIN:
+      //    icmp eq Abs(X) C ->
----------------
pengfei wrote:
> goldstein.w.n wrote:
> > goldstein.w.n wrote:
> > > nikic wrote:
> > > > goldstein.w.n wrote:
> > > > > pengfei wrote:
> > > > > > Seems for `0` and `INT_MIN`, we can simply transform to `icmp eq/ne A, C`. Should we do it together given we already got the constant, or a follow?
> > > > > We can although I don't that that IR ever really reaches the backend as the middle-end would clean up that pattern.
> > > > > My thought it strange IR shouldn't cause broken transforms, but its fine for it to cause missed optimizations.
> > > > > 
> > > > > Although can add those two cases if you'd like, dont really have strong opinions on the matter.
> > > > The general rule of thumb is that DAGCombiner should only copy IR folds if the pattern appears as a result of legalization of canonical IR.
> > > > The general rule of thumb is that DAGCombiner should only copy IR folds if the pattern appears as a result of legalization of canonical IR.
> > > 
> > > @pengfei made: `D142666` but agree with nikic its probably unnecessary. Didn't see any test changes and don't know of any cases where it would reasonably appear.
> > > The general rule of thumb is that DAGCombiner should only copy IR folds if the pattern appears as a result of legalization of canonical IR.
> > 
> > @nikic do you think `InstCombine` should cannocicalize `(or/and (icmp eq/ne X, C), (icmp eq/ne X, -C)) <--> (icmp eq/ne ABS(X), ABS(C))`? I'm sure which was more useful if either.
> Thanks @nikic for the inputs! My comment was inspired when reviewering test cases like `eq_pow_or`, `abs_eq_pow2_zero` etc.
> I did have the same concern, but I'm not clear of it. This also makes me worrying whether the similar thing this patch does is necessary or not and what's important how we check for that? Any pointers would help.
> Thanks @nikic for the inputs! My comment was inspired when reviewering test cases like `eq_pow_or`, `abs_eq_pow2_zero` etc.
> I did have the same concern, but I'm not clear of it. This also makes me worrying whether the similar thing this patch does is necessary or not and what's important how we check for that? Any pointers would help.

I can drop the checks, it still seems to be correct w.o them: https://alive2.llvm.org/ce/z/Q3PTMv

Guess my feeling is we know its the wrong thing to do, and if it does come up, we don't want to be
hiding it. Up to you.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142345