[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:1836
+  if (IsSigned) {
+    auto SExtResult = MIRBuilder.buildSExtInReg(WideTy, Mul, SrcBitWidth);
+    MIRBuilder.buildICmp(CmpInst::ICMP_NE, Overflow, Mul, SExtResult);
----------------
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.



================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3649-3652
+  auto Result = MI.getOperand(0);
+  auto Overflow = MI.getOperand(1);
+  auto LHS = MI.getOperand(2);
+  auto RHS = MI.getOperand(3);
----------------
Should just directly extract the reg here, there's no reason to refer to the MachineOperand. This is also using value copies of MachineOperand, which are generally not a good idea


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3674-3675
+    Register Operand2 = UnmergeRHS->getOperand(I).getReg();
+    Register Result = MRI.createGenericVirtualRegister(ResultTy);
+    Register Overflow = MRI.createGenericVirtualRegister(OverflowTy);
+    MIRBuilder.buildInstr(MI.getOpcode(), {Result, Overflow},
----------------
You can hide the createGenericVirtual register calls with build Instr like buildInstr(Opc, {ResultTy, OverFlowTy}...


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:3683-3684
+  LLT ResultLCMTy = buildLCMMergePieces(SrcTy, NarrowTy, GCDTy, ResultParts);
+  LLT OverflowLCMTy =
+      LLT::scalarOrVector(ResultLCMTy.getNumElements(), LLT::scalar(1));
+
----------------
You should preserve the boolean type of the incoming, not hardcode to s1. We also have LLT.changeElementType for this


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93963