[PATCH] D110890: [GlobalISel] Port the udiv -> mul by constant combine.

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/include/llvm/Target/GlobalISel/Combine.td:692
+def udiv_by_const : GICombineRule<
+  (defs root:$root, apint_matchinfo:$info),
+  (match (wip_match_opcode G_UDIV):$root,
----------------
foad wrote:
> Do you need $info here? It looks unused.
Nope, it's unused.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4429
+/// of the divided value are known zero.
+static mu magicu(const APInt &d, unsigned LeadingZeros = 0) {
+  unsigned p;
----------------
ctetreau wrote:
> foad wrote:
> > aemerson wrote:
> > > This function currently copied but can be shared with the DAG, but I'm waiting for D98200 to land so we can put it in a common utils file.
> > Do you mean D110747?
> This patch is merged now, by the way. I'm glad there's another usage upstream now, so hopefully nobody tries to make it a static local again.
I wasn't aware of that patch actually, but yes I'll use that instead.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4536
+  if (!matchUnaryPredicate(MRI, RHS, BuildUDIVPattern))
+    return nullptr;
+
----------------
foad wrote:
> Can this ever happen? If not, just assert it returns true? Is that what the comment in applyUDivByConst is referring to?
Yes this should just be an assert. The comment was before I refactored some of this.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:4561-4562
+
+    // For vectors we might have a mix of non-NPQ/NPQ paths, so use
+    // G_UMULH to act as a SRL-by-1 for NPQ, else multiply by zero.
+    if (Ty.isVector())
----------------
foad wrote:
> Does the umulh really work out cheaper than (say) shifting NPQ right by one, and then ANDing that with a mask of -1s and 0s?
I think we can implement another combine to optimize the G_MULHU like the DAG does in this case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110890