Skip to content

[AMDGPU] Ensure positive InstOffset for buffer operations #145504

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 4, 2025
Merged

[AMDGPU] Ensure positive InstOffset for buffer operations #145504

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d823e5e
[AMDGPU] Ensure positive InstOffset for buffer operations
aleksandar-amd Jun 24, 2025
ae166cb
[AMDGPU] Fix buffer offset validation per PR feedback
aleksandar-amd Jun 24, 2025
4911278
[AMDGPU] Fix buffer offset validation per PR feedback 2
aleksandar-amd Jun 25, 2025
871d937
[AMDGPU] Fix buffer offset validation per PR feedback 3
aleksandar-amd Jul 18, 2025
ce05d62
[AMDGPU] Resolving conflicts
aleksandar-amd Sep 4, 2025
6bf8bb1
[AMDGPU] Fixed formatting
aleksandar-amd Sep 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4782,12 +4782,14 @@ bool AMDGPUAsmParser::validateOffset(const MCInst &Inst,
return validateSMEMOffset(Inst, Operands);

const auto &Op = Inst.getOperand(OpNum);
// GFX12+ buffer ops: InstOffset is signed 24, but must not be a negative.
if (isGFX12Plus() &&
(TSFlags & (SIInstrFlags::MUBUF | SIInstrFlags::MTBUF))) {
const unsigned OffsetSize = 24;
if (!isIntN(OffsetSize, Op.getImm())) {
if (!isUIntN(OffsetSize - 1, Op.getImm())) {
Error(getFlatOffsetLoc(Operands),
Twine("expected a ") + Twine(OffsetSize) + "-bit signed offset");
Twine("expected a ") + Twine(OffsetSize - 1) +
"-bit unsigned offset for buffer ops");
return false;
}
} else {
Expand Down Expand Up @@ -4870,7 +4872,9 @@ bool AMDGPUAsmParser::validateSMEMOffset(const MCInst &Inst,
return true;

Error(getSMEMOffsetLoc(Operands),
isGFX12Plus() ? "expected a 24-bit signed offset"
isGFX12Plus() && IsBuffer
Copy link
Contributor

@jayfoad jayfoad Jul 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is pretty horrible how we have to guess at the reason for isLegalSMRDEncoded*Offset rejecting our offset. That is not your fault. But maybe we should try to clean it up, as a a separate patch.

Copy link
Contributor Author

@aleksandar-amd aleksandar-amd Jul 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree. Cleaning it up as a separate patch sounds like a great idea.

? "expected a 23-bit unsigned offset for buffer ops"
: isGFX12Plus() ? "expected a 24-bit signed offset"
: (isVI() || IsBuffer) ? "expected a 20-bit unsigned offset"
: "expected a 21-bit signed offset");

Expand Down
26 changes: 26 additions & 0 deletions llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -843,6 +843,18 @@ DecodeStatus AMDGPUDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
}
}

// Validate buffer instruction offsets for GFX12+ - must not be a negative.
if (isGFX12Plus() && isBufferInstruction(MI)) {
int OffsetIdx =
AMDGPU::getNamedOperandIdx(MI.getOpcode(), AMDGPU::OpName::offset);
if (OffsetIdx != -1) {
uint32_t Imm = MI.getOperand(OffsetIdx).getImm();
int64_t SignedOffset = SignExtend64<24>(Imm);
if (SignedOffset < 0)
return MCDisassembler::Fail;
}
}

if (MCII->get(MI.getOpcode()).TSFlags &
(SIInstrFlags::MTBUF | SIInstrFlags::MUBUF)) {
int SWZOpIdx =
Expand Down Expand Up @@ -2772,6 +2784,20 @@ const MCExpr *AMDGPUDisassembler::createConstantSymbolExpr(StringRef Id,
return MCSymbolRefExpr::create(Sym, Ctx);
}

bool AMDGPUDisassembler::isBufferInstruction(const MCInst &MI) const {
const uint64_t TSFlags = MCII->get(MI.getOpcode()).TSFlags;

// Check for MUBUF and MTBUF instructions
if (TSFlags & (SIInstrFlags::MTBUF | SIInstrFlags::MUBUF))
return true;

// Check for SMEM buffer instructions (S_BUFFER_* instructions)
if ((TSFlags & SIInstrFlags::SMRD) && AMDGPU::getSMEMIsBuffer(MI.getOpcode()))
return true;

return false;
}

//===----------------------------------------------------------------------===//
// AMDGPUSymbolizer
//===----------------------------------------------------------------------===//
Expand Down
3 changes: 3 additions & 0 deletions llvm/lib/Target/AMDGPU/Disassembler/AMDGPUDisassembler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,9 @@ class AMDGPUDisassembler : public MCDisassembler {
bool hasKernargPreload() const;

bool isMacDPP(MCInst &MI) const;

/// Check if the instruction is a buffer operation (MUBUF, MTBUF, or S_BUFFER)
bool isBufferInstruction(const MCInst &MI) const;
};

//===----------------------------------------------------------------------===//
Expand Down
5 changes: 4 additions & 1 deletion llvm/lib/Target/AMDGPU/Utils/AMDGPUBaseInfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3220,8 +3220,11 @@ bool isLegalSMRDEncodedUnsignedOffset(const MCSubtargetInfo &ST,

bool isLegalSMRDEncodedSignedOffset(const MCSubtargetInfo &ST,
int64_t EncodedOffset, bool IsBuffer) {
if (isGFX12Plus(ST))
if (isGFX12Plus(ST)) {
if (IsBuffer && EncodedOffset < 0)
return false;
return isInt<24>(EncodedOffset);
}

return !IsBuffer && hasSMRDSignedImmOffset(ST) && isInt<21>(EncodedOffset);
}
Expand Down
Loading