[PATCH] D93963: [GlobalISel][AMDGPU] Lower G_UMULO/G_SMULO

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added a comment.

I think this looks good, just some nits inline. If there are any further improvements they can be done as follow ups.



================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1868
+  // the larger type itself overflowed.
+  if (WideTy.getScalarSizeInBits() < 2 * SrcBitWidth)
+    MIRBuilder.buildOr(OriginalOverflow, Mulo->getOperand(1), Overflow);
----------------
Needs a comment that the multiply can't possible overflow if the wide type is >= 2 * original width.

It's a shame that you have to duplicate this check from line 1845. Maybe @arsenm knows a cleaner way to write this.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3671
+LegalizerHelper::LegalizeResult
+LegalizerHelper::fewerElementsVectorMulo(MachineInstr &MI, unsigned TypeIdx,
+                                         LLT NarrowTy) {
----------------
Personally I would be tempted to generalize LegalizerHelper::fewerElementsVectorMultiEltType into a generic function that can handle any operation that works on vector elements independently. But that does not have to be part of this patch.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1854-1855
+  } else {
+    // Unsigned overflow occurred if the high part is non-zero
+    auto ZExtResult = MIRBuilder.buildZExtInReg(WideTy, Mul, SrcBitWidth);
+    Overflow =
----------------
pdhaliwal wrote:
> arsenm wrote:
> > I don't think the unsigned case is right. The DAG version inserts a shift here, not a mask
> I have changed it use shift instead of masking. Just curious, why was previous logic wrong? I thought zero'ing the upper bits of multiplication result and then comparing it with latter should provide the correct result.
I'm pretty sure the previous logic was fine too, it's just a different way of checking the upper part is zero.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93963