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

[Phoebe Wang via Phabricator via llvm-commits]

pengfei 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 ->
----------------
goldstein.w.n wrote:
> goldstein.w.n wrote:
> > 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.
> > 
> > > 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.
> > 
> 
> Personal preference is:
> leave checks in and ignore the case > handle the case with `D142666` > remove the branches
> 
> 
> 
It looks awkward to me that neither do I want to increase unnecessary code complexity nor can assert these cases never happen.
I don't have preference. I suggest to add comments to explain they are no worth to handle if you want to keep them.

Tips: Phabricator will set the link for it e.g., D142666 if you don't put `` around it.


================
Comment at: llvm/test/CodeGen/X86/icmp-pow2-logic-npow2.ll:11-30
 define i1 @eq_pow_or(i32 %0) nounwind {
 ; X86-LABEL: eq_pow_or:
 ; X86:       # %bb.0:
 ; X86-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; X86-NEXT:    addl $32, %eax
 ; X86-NEXT:    testl $-65, %eax
 ; X86-NEXT:    sete %al
----------------
goldstein.w.n wrote:
> pengfei wrote:
> > So we may remove these tests instead? We should avoid to add dud tests.
> > So we may remove these tests instead? We should avoid to add dud tests.
> 
> This test is for `D142344` where it is not a dud.
> Do you mean `abs_eq_pow2_zero` and `abs_ne_pow2_signed_min`?
Yes. Sorry for the mistake.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142345