[GlobalISel][AMDGPU] Lower G_SMULH/G_UMULH

[pdhaliwal]

removed unneeded changes

[arsenm]

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

1764

no auto.

Can't this use anyext?

1768

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

1769

Use Register, I would worry about introducing a copy of MachineOperand here

1771

LLT not auto

1775

ShiftAmt?

1775

Why isn't the shift amount WideTy.getSizeInBits() - Size? I don't understand - IsSigned

1819

Extra newline

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulh.mir

68

Can you add an 8 and 24-bit test?

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-umulh.mir

109

Can you add an 8 and 24-bit test?

[pdhaliwal]

Review comments

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

1764

I am a bit doubtful if G_ANYEXT would work here. From docs, it doesn't take care of higher bits.

1768

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.

1775

To accomodate the sign bit in case of signed operation.

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulh.mir

68

24-bit case won't work as it requires 48-bit MUL op which is not working yet.

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-umulh.mir

109

Added 8-bit case. But, 24-bit case won't work as it requires 48-bit MUL op which is not working yet.

[foad]

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

6034

I agree that anyext would not work here.

6037

Would this lowering also work for vector types, if you used LLT::scalarOrVector here?

6044

As Matt said you definitely should not subtract IsSigned here.

[pdhaliwal]

Added support for vector types.

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

6044

I got confused in signed binary multiplication. For this operation, it is not required to subtract IsSigned.

[foad]

Looks OK to me modulo one inline comment, as long as Matt has no further objections.

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

6039–6041

Actually it would be neater to use LLT::changeElementSize.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

542 ↗

(On Diff #284762)

The expansion can fully use packed instructions with VOP3P instructions. This should try to clamp the number of elements for 16-bit cases if available before scalarizing

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulh.mir

41

Should add <2 x s16>, <3 x 16> and <4 x s16> cases

[pdhaliwal]

Updated review comments.

[pdhaliwal]

@arsenm , let me know if it is good to land.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

545 ↗

(On Diff #288886)

This isn't the right logic, the intent is to go down to 2 elements for cases that can promote to <2 x i16>. s8 is't special here

[pdhaliwal]

Updated tests and clamping number of elements to 2

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

597 ↗

(On Diff #292461)

This should be unnecessary

[pdhaliwal]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

597 ↗

(On Diff #292461)

If I drop this, the <2 x s32> case starts generating worse code. This is due to lowering coming into the picture which promotes the 32-bit mulh to 64-bit mul and then legalizing 64-bit mul. I can use VOP3P instruction only for S8. For others, I need to specify the scalarization.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

597 ↗

(On Diff #292461)

This should be an unconditional scalarize. The scalarization shouldn't cause a 64-bit multiply to be used

[pdhaliwal]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

597 ↗

(On Diff #292461)

Hmm, unconditional scalarize would remove the possibility of using vector path for <2 x s8>. This is bit different from other operations like MUL, ADD where <2 x s16> would have been legal and unconditional scalarization would have worked. The whole point of having the scalarization conditional is because <2 x s8> can easily use <2 x s16> MUL from lowering path. And as <2 x s16> is legal for AMDGPU, the lowering will correctly use vector operations. Unconditional scalarization would simply make logic of using vector ops void.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

597 ↗

(On Diff #292461)

You already handled this case with the first fewerElementsIf, the second one just handles everything else. It doesn't need to specify not -s8

[pdhaliwal]

Added lowerFor({V2S8})

[pdhaliwal]

Removed unused code

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPULegalizerInfo.cpp

597 ↗

(On Diff #293449)

Put the actions on separate lines

600 ↗

(On Diff #293449)

Separate lines

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-umulh.mir

567

Shouldn't use implicit uses of s8 values. I'm trying to fix implicit uses with illegal register types because we can't ultimately legalize these

[pdhaliwal]

Formatting and removed implicit uses

llvm/include/llvm/CodeGen/GlobalISel/LegalizerHelper.h

private:
   LegalizeResult
   widenScalarUnmergeValues(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
   LegalizeResult
   widenScalarExtract(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
   LegalizeResult
   widenScalarInsert(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
   LegalizeResult
   widenScalarAddSubShlSat(MachineInstr &MI, unsigned TypeIdx, LLT WideTy);
+  LegalizeResult widenScalarAddSubSat(MachineInstr &MI, unsigned TypeIdx,
+                                      LLT WideTy);
+  LegalizeResult widenScalarMulh(MachineInstr &MI, unsigned TypeIdx,
+                                 LLT WideTy);
 
   /// Helper function to split a wide generic register into bitwise blocks with
   /// the given Type (which implies the number of blocks needed). The generic
   /// registers created are appended to Ops, starting at bit 0 of Reg.
   void extractParts(Register Reg, LLT Ty, int NumParts,
                     SmallVectorImpl<Register> &VRegs);
 
   /// Version which handles irregular splits.

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

auto Result = IsSigned ? MIRBuilder.buildAShr(WideTy, WideInst, ShiftK)
                          : MIRBuilder.buildLShr(WideTy, WideInst, ShiftK);
 
   MIRBuilder.buildTrunc(DstReg, Result);
   MI.eraseFromParent();
   return Legalized;
 }
 
 LegalizerHelper::LegalizeResult
+LegalizerHelper::widenScalarMulh(MachineInstr &MI, unsigned TypeIdx,
+                                 LLT WideTy) {
+  bool IsSigned = MI.getOpcode() == TargetOpcode::G_SMULH;
+  auto ExtOp = IsSigned ? TargetOpcode::G_SEXT : TargetOpcode::G_ZEXT;
+
+  auto LHS = MIRBuilder.buildInstr(ExtOp, {WideTy}, {MI.getOperand(1)});
+  auto RHS = MIRBuilder.buildInstr(ExtOp, {WideTy}, {MI.getOperand(2)});
+  auto Mul = MIRBuilder.buildMul(WideTy, LHS, RHS);
+  auto Result = MI.getOperand(0);
+
+  auto Ty = MRI.getType(MI.getOperand(0).getReg());
+  auto Size = Ty.getScalarSizeInBits();
+  auto ShiftOp = IsSigned ? TargetOpcode::G_ASHR : TargetOpcode::G_LSHR;
+
+  auto Shamt = MIRBuilder.buildConstant(WideTy, Size - IsSigned);
+  auto Shifted = MIRBuilder.buildInstr(ShiftOp, {WideTy}, {Mul, Shamt});
+  MIRBuilder.buildTrunc(Result, Shifted);
+
+  MI.eraseFromParent();
+  return Legalized;
+}
+
+LegalizerHelper::LegalizeResult
 LegalizerHelper::widenScalar(MachineInstr &MI, unsigned TypeIdx, LLT WideTy) {
   switch (MI.getOpcode()) {
   default:
     return UnableToLegalize;
   case TargetOpcode::G_EXTRACT:
     return widenScalarExtract(MI, TypeIdx, WideTy);
   case TargetOpcode::G_INSERT:
     return widenScalarInsert(MI, TypeIdx, WideTy);
auto AndOp = MIRBuilder.buildAnd(
         WideTy, NewOp, MIRBuilder.buildConstant(WideTy, Mask));
     // There is no overflow if the AndOp is the same as NewOp.
     MIRBuilder.buildICmp(CmpInst::ICMP_NE, MI.getOperand(1), NewOp, AndOp);
     // Now trunc the NewOp to the original result.
     MIRBuilder.buildTrunc(MI.getOperand(0), NewOp);
     MI.eraseFromParent();
     return Legalized;
   }
+
+  case TargetOpcode::G_UMULH:
+  case TargetOpcode::G_SMULH:
+    return widenScalarMulh(MI, TypeIdx, WideTy);
   case TargetOpcode::G_SADDSAT:
   case TargetOpcode::G_SSUBSAT:
   case TargetOpcode::G_SSHLSAT:
   case TargetOpcode::G_UADDSAT:
   case TargetOpcode::G_USUBSAT:
   case TargetOpcode::G_USHLSAT:
     return widenScalarAddSubShlSat(MI, TypeIdx, WideTy);
   case TargetOpcode::G_CTTZ:
LegalizerHelper::lowerReadWriteRegister(MachineInstr &MI) {
 
   if (IsRead)
     MIRBuilder.buildCopy(ValReg, PhysReg);
   else
     MIRBuilder.buildCopy(PhysReg, ValReg);
 
   MI.eraseFromParent();
   return Legalized;
 }

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-smulh.mir

bb.0:
     ; CHECK: [[SMULH1:%[0-9]+]]:_(s32) = G_SMULH [[UV1]], [[UV3]]
     ; CHECK: [[BUILD_VECTOR:%[0-9]+]]:_(<2 x s32>) = G_BUILD_VECTOR [[SMULH]](s32), [[SMULH1]](s32)
     ; CHECK: $vgpr0_vgpr1 = COPY [[BUILD_VECTOR]](<2 x s32>)
     %0:_(<2 x s32>) = COPY $vgpr0_vgpr1
     %1:_(<2 x s32>) = COPY $vgpr2_vgpr3
     %2:_(<2 x s32>) = G_SMULH %0, %1
     $vgpr0_vgpr1 = COPY %2
 ...
+
+---
+name: test_smulh_s16
+body: |
+  bb.0:
+    liveins: $vgpr0, $vgpr1
+
+    ; CHECK-LABEL: name: test_smulh_s16
+    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $vgpr0
+    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
+    ; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY [[COPY]](s32)
+    ; CHECK: [[SEXT_INREG:%[0-9]+]]:_(s32) = G_SEXT_INREG [[COPY2]], 16
+    ; CHECK: [[COPY3:%[0-9]+]]:_(s32) = COPY [[COPY1]](s32)
+    ; CHECK: [[SEXT_INREG1:%[0-9]+]]:_(s32) = G_SEXT_INREG [[COPY3]], 16
+    ; CHECK: [[MUL:%[0-9]+]]:_(s32) = G_MUL [[SEXT_INREG]], [[SEXT_INREG1]]
+    ; CHECK: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 15
+    ; CHECK: [[ASHR:%[0-9]+]]:_(s32) = G_ASHR [[MUL]], [[C]](s32)
+    ; CHECK: [[COPY4:%[0-9]+]]:_(s32) = COPY [[ASHR]](s32)
+    ; CHECK: [[SEXT_INREG2:%[0-9]+]]:_(s32) = G_SEXT_INREG [[COPY4]], 16
+    ; CHECK: $vgpr0 = COPY [[SEXT_INREG2]](s32)
+    %0:_(s32) = COPY $vgpr0
+    %1:_(s32) = COPY $vgpr1
+    %2:_(s16) = G_TRUNC %0
+    %3:_(s16) = G_TRUNC %1
+    %4:_(s16) = G_SMULH %2, %3
+    %5:_(s32) = G_SEXT %4
+    $vgpr0 = COPY %5
+...

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-umulh.mir

bb.0:
     ; CHECK: [[ADD3:%[0-9]+]]:_(s32) = G_ADD [[UMULH3]], [[ADD2]]
     ; CHECK: [[MV:%[0-9]+]]:_(s64) = G_MERGE_VALUES [[UADDO8]](s32), [[ADD3]](s32)
     ; CHECK: $vgpr0_vgpr1 = COPY [[MV]](s64)
     %0:_(s64) = COPY $vgpr0_vgpr1
     %1:_(s64) = COPY $vgpr2_vgpr3
     %2:_(s64) = G_UMULH %0, %1
     $vgpr0_vgpr1 = COPY %2
 ...
+
+---
+name: test_umulh_s16
+body: |
+  bb.0:
+    liveins: $vgpr0, $vgpr1
+
+    ; CHECK-LABEL: name: test_umulh_s16
+    ; CHECK: [[COPY:%[0-9]+]]:_(s32) = COPY $vgpr0
+    ; CHECK: [[COPY1:%[0-9]+]]:_(s32) = COPY $vgpr1
+    ; CHECK: [[C:%[0-9]+]]:_(s32) = G_CONSTANT i32 65535
+    ; CHECK: [[COPY2:%[0-9]+]]:_(s32) = COPY [[COPY]](s32)
+    ; CHECK: [[AND:%[0-9]+]]:_(s32) = G_AND [[COPY2]], [[C]]
+    ; CHECK: [[COPY3:%[0-9]+]]:_(s32) = COPY [[COPY1]](s32)
+    ; CHECK: [[AND1:%[0-9]+]]:_(s32) = G_AND [[COPY3]], [[C]]
+    ; CHECK: [[MUL:%[0-9]+]]:_(s32) = G_MUL [[AND]], [[AND1]]
+    ; CHECK: [[C1:%[0-9]+]]:_(s32) = G_CONSTANT i32 16
+    ; CHECK: [[LSHR:%[0-9]+]]:_(s32) = G_LSHR [[MUL]], [[C1]](s32)
+    ; CHECK: [[COPY4:%[0-9]+]]:_(s32) = COPY [[LSHR]](s32)
+    ; CHECK: [[AND2:%[0-9]+]]:_(s32) = G_AND [[COPY4]], [[C]]
+    ; CHECK: $vgpr0 = COPY [[AND2]](s32)
+    %0:_(s32) = COPY $vgpr0
+    %1:_(s32) = COPY $vgpr1
+    %2:_(s16) = G_TRUNC %0
+    %3:_(s16) = G_TRUNC %1
+    %4:_(s16) = G_UMULH %2, %3
+    %5:_(s32) = G_ZEXT %4
+    $vgpr0 = COPY %5
+...