[PATCH] D130517: [GlobalISel] Add sdiv exact (X, constant) -> mul combine.

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4944
+  auto *RHSDef = MRI.getVRegDef(RHS);
+  if (!isConstantOrConstantVector(*RHSDef, MRI))
+    return false;
----------------
foad wrote:
> This seems redundant given the call to matchUnaryPredicate below. (I don't think it's significantly cheaper than matchUnaryPredicate is it?)
Yeah I think you're right.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:5013
+    unsigned W = Divisor.getBitWidth();
+    APInt Factor = Divisor.zext(W + 1)
+                       .multiplicativeInverse(APInt::getSignedMinValue(W + 1))
----------------
foad wrote:
> Is zero extending really correct here? It feels dubious for a signed division but I'm not sure of the maths at this point.
This is also getting beyond the realms on my expertise, but the documentation for it says that the inverse should always fit into the bit width, so extending and truncating shouldn’t affect the result in anyway.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:5005
+
+    // Calculate the multiplicative inverse, using Newton's method.
+    APInt t;
----------------
foad wrote:
> foad wrote:
> > Use APInt::multiplicativeInverse instead?
> Ping. Is there a reason not to use it?
I did below?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130517