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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1832
+
+  auto Mul = MIRBuilder.buildMul(WideTy, LeftOperand, RightOperand);
+  MIRBuilder.buildTrunc(Result, Mul);
----------------
Hmm. This is actually going beyond what I expect widenScalar to do. In general widenScalar should try to produce the original opcode. In this case, the DAG does the same overflow opcode in the wider type, and then ors the flag at the end.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1842-1845
+    auto Mask = MIRBuilder.buildConstant(
+        WideTy,
+        APInt::getLowBitsSet(WideTy.getScalarSizeInBits(), SrcBitWidth));
+    auto And = MIRBuilder.buildAnd(WideTy, Mul, Mask);
----------------
This is a common enough pattern. The DAG provides a getZExtInReg to help produce masks like this, maybe move this to MIRBuilder?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1836
+  if (IsSigned) {
+    auto SExtResult = MIRBuilder.buildSExtInReg(WideTy, Mul, SrcBitWidth);
+    MIRBuilder.buildICmp(CmpInst::ICMP_NE, Overflow, Mul, SExtResult);
----------------
arsenm wrote:
> Can you copy some of the comments from the DAG version? Stuff like    
> // Unsigned overflow occurred if the high part is non-zero.
> and    
> 
> // Signed overflow occurred if the high part does not sign extend the low.
> 
The DAG version still has a few more comments ( To determine if the result overflowed in a larger type...)



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93963