[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 ->
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142345