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

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


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


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:5013
+    unsigned W = Divisor.getBitWidth();
+    APInt Factor = Divisor.zext(W + 1)
+                       .multiplicativeInverse(APInt::getSignedMinValue(W + 1))
----------------
Is zero extending really correct here? It feels dubious for a signed division but I'm not sure of the maths at this point.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130517