[AMDGPU] Fix folding of v2i16/v2f16 splat imms #72709
Conversation
We can use inline constants with packed 16-bit operands, but these should use op_sel. Currently splat of inlinable constants is considered legal, which is not really true if we fail to fold it with op_sel and drop the high half. It may be legal as a literal but not as inline constant, but then usual literal checks must be performed. This patch makes these splat literals illegal but adds additional logic to the operand folding to keep current folds. This logic is somewhat heavy though. This has fixed two bugs: constant bus violation in the fdot2 test and invalid selection of inline constant 1 without op_sel in the udot2 test.
|
@llvm/pr-subscribers-backend-amdgpu Author: Stanislav Mekhanoshin (rampitec) ChangesWe can use inline constants with packed 16-bit operands, but these should use op_sel. Currently splat of inlinable constants is considered legal, which is not really true if we fail to fold it with op_sel and drop the high half. It may be legal as a literal but not as inline constant, but then usual literal checks must be performed. This patch makes these splat literals illegal but adds additional logic to the operand folding to keep current folds. This logic is somewhat heavy though. This has fixed two bugs: constant bus violation in the fdot2 test and invalid selection of inline constant 1 without op_sel in the udot2 test. Full diff: https://github.com/llvm/llvm-project/pull/72709.diff 6 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
index 0ec0370e21dfc16..709de612d81d4a1 100644
--- a/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
+++ b/llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
@@ -80,6 +80,10 @@ class SIFoldOperands : public MachineFunctionPass {
bool updateOperand(FoldCandidate &Fold) const;
+ bool canUseImmWithOpSel(FoldCandidate &Fold) const;
+
+ bool tryFoldImmWithOpSel(FoldCandidate &Fold) const;
+
bool tryAddToFoldList(SmallVectorImpl<FoldCandidate> &FoldList,
MachineInstr *MI, unsigned OpNo,
MachineOperand *OpToFold) const;
@@ -196,60 +200,85 @@ FunctionPass *llvm::createSIFoldOperandsPass() {
return new SIFoldOperands();
}
-bool SIFoldOperands::updateOperand(FoldCandidate &Fold) const {
+bool SIFoldOperands::canUseImmWithOpSel(FoldCandidate &Fold) const {
MachineInstr *MI = Fold.UseMI;
MachineOperand &Old = MI->getOperand(Fold.UseOpNo);
- assert(Old.isReg());
+ const uint64_t TSFlags = MI->getDesc().TSFlags;
+ assert(Old.isReg() && Fold.isImm());
- const uint64_t TSFlags = MI->getDesc().TSFlags;
- if (Fold.isImm()) {
- if (TSFlags & SIInstrFlags::IsPacked && !(TSFlags & SIInstrFlags::IsMAI) &&
- (!ST->hasDOTOpSelHazard() || !(TSFlags & SIInstrFlags::IsDOT)) &&
- AMDGPU::isFoldableLiteralV216(Fold.ImmToFold,
- ST->hasInv2PiInlineImm())) {
- // Set op_sel/op_sel_hi on this operand or bail out if op_sel is
- // already set.
- unsigned Opcode = MI->getOpcode();
- int OpNo = MI->getOperandNo(&Old);
- int ModIdx = -1;
- if (OpNo == AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src0))
- ModIdx = AMDGPU::OpName::src0_modifiers;
- else if (OpNo == AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src1))
- ModIdx = AMDGPU::OpName::src1_modifiers;
- else if (OpNo == AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src2))
- ModIdx = AMDGPU::OpName::src2_modifiers;
- assert(ModIdx != -1);
- ModIdx = AMDGPU::getNamedOperandIdx(Opcode, ModIdx);
- MachineOperand &Mod = MI->getOperand(ModIdx);
- unsigned Val = Mod.getImm();
- if (!(Val & SISrcMods::OP_SEL_0) && (Val & SISrcMods::OP_SEL_1)) {
- // Only apply the following transformation if that operand requires
- // a packed immediate.
- switch (TII->get(Opcode).operands()[OpNo].OperandType) {
- case AMDGPU::OPERAND_REG_IMM_V2FP16:
- case AMDGPU::OPERAND_REG_IMM_V2INT16:
- case AMDGPU::OPERAND_REG_INLINE_C_V2FP16:
- case AMDGPU::OPERAND_REG_INLINE_C_V2INT16:
- // If upper part is all zero we do not need op_sel_hi.
- if (!isUInt<16>(Fold.ImmToFold)) {
- if (!(Fold.ImmToFold & 0xffff)) {
- Mod.setImm(Mod.getImm() | SISrcMods::OP_SEL_0);
- Mod.setImm(Mod.getImm() & ~SISrcMods::OP_SEL_1);
- Old.ChangeToImmediate((Fold.ImmToFold >> 16) & 0xffff);
- return true;
- }
- Mod.setImm(Mod.getImm() & ~SISrcMods::OP_SEL_1);
- Old.ChangeToImmediate(Fold.ImmToFold & 0xffff);
- return true;
- }
- break;
- default:
- break;
- }
- }
- }
+ if (!(TSFlags & SIInstrFlags::IsPacked) || (TSFlags & SIInstrFlags::IsMAI) ||
+ (ST->hasDOTOpSelHazard() && (TSFlags & SIInstrFlags::IsDOT)) ||
+ isUInt<16>(Fold.ImmToFold) ||
+ !AMDGPU::isFoldableLiteralV216(Fold.ImmToFold, ST->hasInv2PiInlineImm()))
+ return false;
+
+ unsigned Opcode = MI->getOpcode();
+ int OpNo = MI->getOperandNo(&Old);
+ uint8_t OpType = TII->get(Opcode).operands()[OpNo].OperandType;
+ switch (OpType) {
+ default:
+ return false;
+ case AMDGPU::OPERAND_REG_IMM_V2FP16:
+ case AMDGPU::OPERAND_REG_IMM_V2INT16:
+ case AMDGPU::OPERAND_REG_INLINE_C_V2FP16:
+ case AMDGPU::OPERAND_REG_INLINE_C_V2INT16:
+ break;
+ }
+
+ return true;
+}
+
+bool SIFoldOperands::tryFoldImmWithOpSel(FoldCandidate &Fold) const {
+ MachineInstr *MI = Fold.UseMI;
+ MachineOperand &Old = MI->getOperand(Fold.UseOpNo);
+ unsigned Opcode = MI->getOpcode();
+ int OpNo = MI->getOperandNo(&Old);
+
+ // Set op_sel/op_sel_hi on this operand or bail out if op_sel is
+ // already set.
+ int ModIdx = -1;
+ if (OpNo == AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src0))
+ ModIdx = AMDGPU::OpName::src0_modifiers;
+ else if (OpNo == AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src1))
+ ModIdx = AMDGPU::OpName::src1_modifiers;
+ else if (OpNo == AMDGPU::getNamedOperandIdx(Opcode, AMDGPU::OpName::src2))
+ ModIdx = AMDGPU::OpName::src2_modifiers;
+ assert(ModIdx != -1);
+ ModIdx = AMDGPU::getNamedOperandIdx(Opcode, ModIdx);
+ MachineOperand &Mod = MI->getOperand(ModIdx);
+ unsigned Val = Mod.getImm();
+ if ((Val & SISrcMods::OP_SEL_0) || !(Val & SISrcMods::OP_SEL_1))
+ return false;
+
+ // Only apply the following transformation if that operand requires
+ // a packed immediate.
+ // If upper part is all zero we do not need op_sel_hi.
+ if (!(Fold.ImmToFold & 0xffff)) {
+ MachineOperand New =
+ MachineOperand::CreateImm((Fold.ImmToFold >> 16) & 0xffff);
+ if (!TII->isOperandLegal(*MI, OpNo, &New))
+ return false;
+ Mod.setImm(Mod.getImm() | SISrcMods::OP_SEL_0);
+ Mod.setImm(Mod.getImm() & ~SISrcMods::OP_SEL_1);
+ Old.ChangeToImmediate((Fold.ImmToFold >> 16) & 0xffff);
+ return true;
}
+ MachineOperand New = MachineOperand::CreateImm(Fold.ImmToFold & 0xffff);
+ if (!TII->isOperandLegal(*MI, OpNo, &New))
+ return false;
+ Mod.setImm(Mod.getImm() & ~SISrcMods::OP_SEL_1);
+ Old.ChangeToImmediate(Fold.ImmToFold & 0xffff);
+ return true;
+}
+
+bool SIFoldOperands::updateOperand(FoldCandidate &Fold) const {
+ MachineInstr *MI = Fold.UseMI;
+ MachineOperand &Old = MI->getOperand(Fold.UseOpNo);
+ assert(Old.isReg());
+
+ if (Fold.isImm() && canUseImmWithOpSel(Fold))
+ return tryFoldImmWithOpSel(Fold);
if ((Fold.isImm() || Fold.isFI() || Fold.isGlobal()) && Fold.needsShrink()) {
MachineBasicBlock *MBB = MI->getParent();
@@ -381,7 +410,13 @@ bool SIFoldOperands::tryAddToFoldList(SmallVectorImpl<FoldCandidate> &FoldList,
return false;
};
- if (!TII->isOperandLegal(*MI, OpNo, OpToFold)) {
+ bool IsLegal = TII->isOperandLegal(*MI, OpNo, OpToFold);
+ if (!IsLegal && OpToFold->isImm()) {
+ FoldCandidate Fold(MI, OpNo, OpToFold);
+ IsLegal = canUseImmWithOpSel(Fold);
+ }
+
+ if (!IsLegal) {
// Special case for v_mac_{f16, f32}_e64 if we are trying to fold into src2
unsigned NewOpc = macToMad(Opc);
if (NewOpc != AMDGPU::INSTRUCTION_LIST_END) {
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index c4baabcd9232b56..5498624fb4cca72 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -4149,12 +4149,15 @@ bool SIInstrInfo::isInlineConstant(const MachineOperand &MO,
case AMDGPU::OPERAND_REG_IMM_V2INT16:
case AMDGPU::OPERAND_REG_INLINE_C_V2INT16:
case AMDGPU::OPERAND_REG_INLINE_AC_V2INT16:
- // This suffers the same problem as the scalar 16-bit cases.
- return AMDGPU::isInlinableIntLiteralV216(Imm);
+ return (isInt<16>(Imm) || isUInt<16>(Imm)) &&
+ AMDGPU::isInlinableIntLiteral((int16_t)Imm);
case AMDGPU::OPERAND_REG_IMM_FP16:
case AMDGPU::OPERAND_REG_IMM_FP16_DEFERRED:
case AMDGPU::OPERAND_REG_INLINE_C_FP16:
- case AMDGPU::OPERAND_REG_INLINE_AC_FP16: {
+ case AMDGPU::OPERAND_REG_INLINE_AC_FP16:
+ case AMDGPU::OPERAND_REG_IMM_V2FP16:
+ case AMDGPU::OPERAND_REG_INLINE_C_V2FP16:
+ case AMDGPU::OPERAND_REG_INLINE_AC_V2FP16: {
if (isInt<16>(Imm) || isUInt<16>(Imm)) {
// A few special case instructions have 16-bit operands on subtargets
// where 16-bit instructions are not legal.
@@ -4167,12 +4170,6 @@ bool SIInstrInfo::isInlineConstant(const MachineOperand &MO,
return false;
}
- case AMDGPU::OPERAND_REG_IMM_V2FP16:
- case AMDGPU::OPERAND_REG_INLINE_C_V2FP16:
- case AMDGPU::OPERAND_REG_INLINE_AC_V2FP16: {
- uint32_t Trunc = static_cast<uint32_t>(Imm);
- return AMDGPU::isInlinableLiteralV216(Trunc, ST.hasInv2PiInlineImm());
- }
case AMDGPU::OPERAND_KIMM32:
case AMDGPU::OPERAND_KIMM16:
return false;
diff --git a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
index a09abc639d7590f..6e57179a2c5621e 100644
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
@@ -2487,6 +2487,16 @@ bool isInlinableIntLiteralV216(int32_t Literal) {
return Lo16 == Hi16 && isInlinableIntLiteral(Lo16);
}
+bool isInlinableLiteralV216(int32_t Literal, bool HasInv2Pi, uint8_t OpType) {
+ switch (OpType) {
+ case AMDGPU::OPERAND_REG_IMM_V2FP16:
+ case AMDGPU::OPERAND_REG_INLINE_C_V2FP16:
+ return isInlinableLiteralV216(Literal, HasInv2Pi);
+ default:
+ return isInlinableIntLiteralV216(Literal);
+ }
+}
+
bool isFoldableLiteralV216(int32_t Literal, bool HasInv2Pi) {
assert(HasInv2Pi);
diff --git a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
index 1e0994d0862cf5d..81613bad0514d18 100644
--- a/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
+++ b/llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.h
@@ -1280,6 +1280,9 @@ bool isInlinableLiteralV216(int32_t Literal, bool HasInv2Pi);
LLVM_READNONE
bool isInlinableIntLiteralV216(int32_t Literal);
+LLVM_READNONE
+bool isInlinableLiteralV216(int32_t Literal, bool HasInv2Pi, uint8_t OpType);
+
LLVM_READNONE
bool isFoldableLiteralV216(int32_t Literal, bool HasInv2Pi);
diff --git a/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.fdot2.bf16.bf16.ll b/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.fdot2.bf16.bf16.ll
index 3d3bf7bbb54f9a3..54bd78e2ea12702 100644
--- a/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.fdot2.bf16.bf16.ll
+++ b/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.fdot2.bf16.bf16.ll
@@ -72,16 +72,29 @@ entry:
ret void
}
-; FIXME: This test violates constant bus restriction.
+; Make sure we do not violate constant bus restriction with 3 scalar inputs and simingly inlinable literal.
define amdgpu_ps void @test_llvm_amdgcn_fdot2_bf16_bf16_sis(
-; GFX11-LABEL: test_llvm_amdgcn_fdot2_bf16_bf16_sis:
-; GFX11: ; %bb.0: ; %entry
-; GFX11-NEXT: v_dot2_bf16_bf16 v2, s0, 0x10001, s1
-; GFX11-NEXT: global_store_b16 v[0:1], v2, off
-; GFX11-NEXT: s_nop 0
-; GFX11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
-; GFX11-NEXT: s_endpgm
+; SDAG-GFX11-LABEL: test_llvm_amdgcn_fdot2_bf16_bf16_sis:
+; SDAG-GFX11: ; %bb.0: ; %entry
+; SDAG-GFX11-NEXT: v_mov_b32_e32 v2, s1
+; SDAG-GFX11-NEXT: s_mov_b32 s1, 0x10001
+; SDAG-GFX11-NEXT: s_delay_alu instid0(VALU_DEP_1) | instid1(SALU_CYCLE_1)
+; SDAG-GFX11-NEXT: v_dot2_bf16_bf16 v2, s0, s1, v2
+; SDAG-GFX11-NEXT: global_store_b16 v[0:1], v2, off
+; SDAG-GFX11-NEXT: s_nop 0
+; SDAG-GFX11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
+; SDAG-GFX11-NEXT: s_endpgm
+;
+; GISEL-GFX11-LABEL: test_llvm_amdgcn_fdot2_bf16_bf16_sis:
+; GISEL-GFX11: ; %bb.0: ; %entry
+; GISEL-GFX11-NEXT: v_mov_b32_e32 v2, 0x10001
+; GISEL-GFX11-NEXT: s_delay_alu instid0(VALU_DEP_1)
+; GISEL-GFX11-NEXT: v_dot2_bf16_bf16 v2, s0, v2, s1
+; GISEL-GFX11-NEXT: global_store_b16 v[0:1], v2, off
+; GISEL-GFX11-NEXT: s_nop 0
+; GISEL-GFX11-NEXT: s_sendmsg sendmsg(MSG_DEALLOC_VGPRS)
+; GISEL-GFX11-NEXT: s_endpgm
ptr addrspace(1) %r,
<2 x i16> inreg %a,
i16 inreg %c) {
diff --git a/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.udot2.ll b/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.udot2.ll
index 490ce706455cd6f..1c0def2a4f8de04 100644
--- a/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.udot2.ll
+++ b/llvm/test/CodeGen/AMDGPU/llvm.amdgcn.udot2.ll
@@ -41,9 +41,11 @@ entry:
ret void
}
+; FIXME: GFX940 fails to fold 0x1001 with op_sel
+
; GCN-LABEL: {{^}}test_llvm_amdgcn_udot2_op_sel:
; GFX906: v_dot2_u32_u16 v{{[0-9]+}}, 1, v{{[0-9]+}}, s{{[0-9]+}} op_sel:[0,1,0] op_sel_hi:[0,0,1]{{$}}
-; GFX940: v_dot2_u32_u16 v{{[0-9]+}}, 1, v{{[0-9]+}}, s{{[0-9]+}}{{$}}
+; GFX940: v_dot2_u32_u16 v{{[0-9]+}}, {{[sv][0-9]+}}, v{{[0-9]+}}, {{[sv][0-9]+}}{{$}}
; GFX10: v_dot2_u32_u16 v{{[0-9]+}}, 1, v{{[0-9]+}}, s{{[0-9]+}} op_sel:[0,1,0] op_sel_hi:[0,0,1]{{$}}
define amdgpu_kernel void @test_llvm_amdgcn_udot2_op_sel(
ptr addrspace(1) %r,
|
The docs say that there are special rules for this for f16/bf16 dot2 instructions - see the RDNA3 Instruction Set Architecture So does this code need to treat f16/bf16 dot2 instructions differently from other packed instructions? |
This needs a knowledge of the instruction to decide if a constant is legal or not, and the operand number. The whole infrastructure does not know it. Moreover, it does not help the current constant bus violation, it is still a violation because inline constant is not used. When this is fixed one could potentially use this picularity (only and only on gfx11), although I do not know why. Using op_sel does not make anything worse, so I see no single reason to distinguish instructions and their operand number for that reason. |
In addition ISA reference is weird here. It says that for src2 OPSEL shall be used to control replication, but src2 is not packed, instructions only read low 16 bit of it. Anyhow, to me the issue of low bits auto replication and emission of invalid 32-bit literal thinking it is an inline immediate are orthogonal. |
|
And then this only happens with BF16, for I16 and F16 we do not emit literal thinking it is an inline constant even with the existing code. And the inline constant handling for BF16 seems to be completely broken because we think of it as of either I16 or F16 and use those values, while the same manual says: So this maybe have to be replaced with special operand types for BF16/2xBF16 accepting fp32 bit inline constants in packed form and not accepting literals in any form. For instance this 0x10001 may not be what we think it is even if the literal is legal here. I have found this description of the inline constants handling in this case, but I do not see a description of what happens with actual literals either in packed or scalar form. |
|
Decoupled fix for gfx940 udot: #73589 |
|
After some digging I believe with this bug fixed we are fine now. Since we are passing all bf16 inputs as i16 we can only inline small integers, and inline integer 1 shall be the same as using 1 in an input register I believe. Although we are missing a potential optimization, say we could fold 'i16 0x3f80' as inline constant 1.0, and a pair of these as 1.0 with opsel should we know this is really a bf16 operand. |
We can use inline constants with packed 16-bit operands, but these should use op_sel. Currently splat of inlinable constants is considered legal, which is not really true if we fail to fold it with op_sel and drop the high half. It may be legal as a literal but not as inline constant, but then usual literal checks must be performed.
This patch makes these splat literals illegal but adds additional logic to the operand folding to keep current folds. This logic is somewhat heavy though.
This has fixed constant bus violation in the fdot2 test.