[PATCH] D85653: [GlobalISel] widenScalar G_SMULH/G_UMULH

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1764
+  bool IsSigned = MI.getOpcode() == TargetOpcode::G_SMULH;
+  auto ExtOp = IsSigned ? TargetOpcode::G_SEXT : TargetOpcode::G_ZEXT;
+
----------------
no auto.

Can't this use anyext?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1768
+  auto RHS = MIRBuilder.buildInstr(ExtOp, {WideTy}, {MI.getOperand(2)});
+  auto Mul = MIRBuilder.buildMul(WideTy, LHS, RHS);
+  auto Result = MI.getOperand(0);
----------------
Something seems off to me about introducing a full multiply, and in whatever type the user requested. I think this only works if WideTy == 2 * OriginalType. Can you produce a mulh in the wider type? This seems more like a lowering


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1769
+  auto Mul = MIRBuilder.buildMul(WideTy, LHS, RHS);
+  auto Result = MI.getOperand(0);
+
----------------
Use Register, I would worry about introducing a copy of MachineOperand here


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1771
+
+  auto Ty = MRI.getType(MI.getOperand(0).getReg());
+  auto Size = Ty.getScalarSizeInBits();
----------------
LLT not auto


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1775
+
+  auto Shamt = MIRBuilder.buildConstant(WideTy, Size - IsSigned);
+  auto Shifted = MIRBuilder.buildInstr(ShiftOp, {WideTy}, {Mul, Shamt});
----------------
ShiftAmt?


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1775
+
+  auto Shamt = MIRBuilder.buildConstant(WideTy, Size - IsSigned);
+  auto Shifted = MIRBuilder.buildInstr(ShiftOp, {WideTy}, {Mul, Shamt});
----------------
arsenm wrote:
> ShiftAmt?
Why isn't the shift amount WideTy.getSizeInBits() - Size? I don't understand - IsSigned


================
Comment at: llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp:1819
   }
+
+  case TargetOpcode::G_UMULH:
----------------
Extra newline


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulh.mir:68
+    $vgpr0 = COPY %5
+...
----------------
Can you add an 8 and 24-bit test?


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-umulh.mir:109
+    $vgpr0 = COPY %5
+...
----------------
Can you add an 8 and 24-bit test?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85653