[PATCH] D67087: [X86] Override BuildSDIVPow2 for X86.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

We should add the minimal/motivating test from PR43197, so we cover that exact case, and we should have at least 1 test with a negative divisor constant rather than relying on srem transforms to get that case.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:20109-20112
+  if (!Subtarget.hasCMov() ||
+      (VT != MVT::i32 && !(Subtarget.is64Bit() && VT == MVT::i64)) ||
+      !(Divisor.isPowerOf2() || (-Divisor).isPowerOf2()) ||
+      Divisor.isMinSignedValue())
----------------
That's a complicated string of logic. It would be easier to read if it was split into 2-3 clauses:
1. Check target capability
2. Check type
3. Check divisor value


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:20112
+      !(Divisor.isPowerOf2() || (-Divisor).isPowerOf2()) ||
+      Divisor.isMinSignedValue())
+    return SDValue();
----------------
The min-signed check is not necessary?
https://rise4fun.com/Alive/Ki68

  Name: sdiv X, {+/-} power-of-2-constant
  Pre: isPowerOf2(C1) || isPowerOf2(-C1)
  %r = sdiv i32 %x, C1
  =>
  %x_is_negative = icmp slt i32 %x, 0
  %x_with_offset = add i32 %x, ((1 << countTrailingZeros(C1)) - 1)
  %normx = select i1 %x_is_negative, i32 %x_with_offset, i32 %x
  %a = ashr i32 %normx, countTrailingZeros(C1)
  %neg = sub i32 0, %a
  %divisor_is_negative = icmp slt i32 C1, 0 ; constant fold
  %r = select i1 %divisor_is_negative, i32 %neg, %a


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:20121
+
+  // Add (N0 < 0) ? Pow2 - 1 : 0;
+  SDValue Cmp = DAG.getSetCC(DL, MVT::i8, N0, Zero, ISD::SETLT);
----------------
This comment doesn't match what we're creating - the 'add' is the true operand of the select:
 // (N0 < 0) ? (N0 + Pow2M1) : N0


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67087