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

[Pushpinder Singh via Phabricator via llvm-commits]

pdhaliwal 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;
+
----------------
arsenm wrote:
> no auto.
> 
> Can't this use anyext?
I am a bit doubtful if G_ANYEXT would work here. From docs, it doesn't take care of [[ https://llvm.org/docs/GlobalISel/GenericOpcode.html#g-anyext | higher bits ]]. 


================
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);
----------------
arsenm wrote:
> 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
Yes, it would only work when WideTy == 2 * OriginalType.  And now if I think again it is more of a lowering operation than widening as user is not always free to choose the wider type.


================
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:
> arsenm wrote:
> > ShiftAmt?
> Why isn't the shift amount WideTy.getSizeInBits() - Size? I don't understand - IsSigned
To accomodate the sign bit in case of signed operation.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulh.mir:68
+    $vgpr0 = COPY %5
+...
----------------
arsenm wrote:
> Can you add an 8 and 24-bit test?
24-bit case won't work as it requires 48-bit MUL op which  is not working yet.


================
Comment at: llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-umulh.mir:109
+    $vgpr0 = COPY %5
+...
----------------
arsenm wrote:
> Can you add an 8 and 24-bit test?
Added 8-bit case. But, 24-bit case won't work as it requires 48-bit MUL op which is not working yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85653