-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
[AMDGPU] Fix negative immediate offset for unbuffered smem loads #89165
Changes from 4 commits
15a34d5
c369787
7ce94de
2ec61da
e50ac18
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1980,28 +1980,50 @@ bool AMDGPUDAGToDAGISel::SelectScratchSVAddr(SDNode *N, SDValue Addr, | |
return true; | ||
} | ||
|
||
// For unbuffered smem loads, it is illegal for the Immediate Offset to be | ||
// negative if the resulting (Offset + (M0 or SOffset or zero) is negative. | ||
// Handle the case where the Immediate Offset + SOffset is negative. | ||
bool AMDGPUDAGToDAGISel::isSOffsetLegalWithImmOffset(SDValue *SOffset, | ||
bool Imm32Only, | ||
bool IsBuffer, | ||
int64_t ImmOffset) const { | ||
if (!IsBuffer && !Imm32Only && ImmOffset < 0 && | ||
AMDGPU::hasSMRDSignedImmOffset(*Subtarget)) { | ||
KnownBits SKnown = CurDAG->computeKnownBits(*SOffset); | ||
if (ImmOffset + SKnown.getMinValue().getSExtValue() < 0) | ||
return false; | ||
} | ||
|
||
return true; | ||
} | ||
|
||
// Match an immediate (if Offset is not null) or an SGPR (if SOffset is | ||
// not null) offset. If Imm32Only is true, match only 32-bit immediate | ||
// offsets available on CI. | ||
bool AMDGPUDAGToDAGISel::SelectSMRDOffset(SDValue ByteOffsetNode, | ||
SDValue *SOffset, SDValue *Offset, | ||
bool Imm32Only, bool IsBuffer) const { | ||
bool Imm32Only, bool IsBuffer, | ||
bool HasSOffset, | ||
int64_t ImmOffset) const { | ||
assert((!SOffset || !Offset) && | ||
"Cannot match both soffset and offset at the same time!"); | ||
|
||
ConstantSDNode *C = dyn_cast<ConstantSDNode>(ByteOffsetNode); | ||
if (!C) { | ||
if (!SOffset) | ||
return false; | ||
|
||
if (ByteOffsetNode.getValueType().isScalarInteger() && | ||
ByteOffsetNode.getValueType().getSizeInBits() == 32) { | ||
*SOffset = ByteOffsetNode; | ||
return true; | ||
return isSOffsetLegalWithImmOffset(SOffset, Imm32Only, IsBuffer, | ||
ImmOffset); | ||
} | ||
if (ByteOffsetNode.getOpcode() == ISD::ZERO_EXTEND) { | ||
if (ByteOffsetNode.getOperand(0).getValueType().getSizeInBits() == 32) { | ||
*SOffset = ByteOffsetNode.getOperand(0); | ||
return true; | ||
return isSOffsetLegalWithImmOffset(SOffset, Imm32Only, IsBuffer, | ||
ImmOffset); | ||
} | ||
} | ||
return false; | ||
|
@@ -2012,8 +2034,8 @@ bool AMDGPUDAGToDAGISel::SelectSMRDOffset(SDValue ByteOffsetNode, | |
// GFX9 and GFX10 have signed byte immediate offsets. The immediate | ||
// offset for S_BUFFER instructions is unsigned. | ||
int64_t ByteOffset = IsBuffer ? C->getZExtValue() : C->getSExtValue(); | ||
std::optional<int64_t> EncodedOffset = | ||
AMDGPU::getSMRDEncodedOffset(*Subtarget, ByteOffset, IsBuffer); | ||
std::optional<int64_t> EncodedOffset = AMDGPU::getSMRDEncodedOffset( | ||
*Subtarget, ByteOffset, IsBuffer, HasSOffset); | ||
if (EncodedOffset && Offset && !Imm32Only) { | ||
*Offset = CurDAG->getTargetConstant(*EncodedOffset, SL, MVT::i32); | ||
return true; | ||
|
@@ -2072,13 +2094,22 @@ SDValue AMDGPUDAGToDAGISel::Expand32BitAddress(SDValue Addr) const { | |
// true, match only 32-bit immediate offsets available on CI. | ||
bool AMDGPUDAGToDAGISel::SelectSMRDBaseOffset(SDValue Addr, SDValue &SBase, | ||
SDValue *SOffset, SDValue *Offset, | ||
bool Imm32Only, | ||
bool IsBuffer) const { | ||
bool Imm32Only, bool IsBuffer, | ||
bool HasSOffset, | ||
int64_t ImmOffset) const { | ||
if (SOffset && Offset) { | ||
assert(!Imm32Only && !IsBuffer); | ||
SDValue B; | ||
return SelectSMRDBaseOffset(Addr, B, nullptr, Offset) && | ||
SelectSMRDBaseOffset(B, SBase, SOffset, nullptr); | ||
|
||
if (!SelectSMRDBaseOffset(Addr, B, nullptr, Offset, false, false, true)) | ||
return false; | ||
|
||
int64_t ImmOff = 0; | ||
if (ConstantSDNode *C = dyn_cast<ConstantSDNode>(*Offset)) | ||
ImmOff = C->getSExtValue(); | ||
Comment on lines
+2107
to
+2109
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Parsing out the constant offset here is harder to follow, SelectSMRDBaseOffset can do this just as well There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reiterating this, you're splitting up the offset parsing up |
||
|
||
return SelectSMRDBaseOffset(B, SBase, SOffset, nullptr, false, false, true, | ||
ImmOff); | ||
} | ||
|
||
// A 32-bit (address + offset) should not cause unsigned 32-bit integer | ||
|
@@ -2097,11 +2128,14 @@ bool AMDGPUDAGToDAGISel::SelectSMRDBaseOffset(SDValue Addr, SDValue &SBase, | |
} | ||
if (!N0 || !N1) | ||
return false; | ||
if (SelectSMRDOffset(N1, SOffset, Offset, Imm32Only, IsBuffer)) { | ||
|
||
if (SelectSMRDOffset(N1, SOffset, Offset, Imm32Only, IsBuffer, HasSOffset, | ||
ImmOffset)) { | ||
SBase = N0; | ||
return true; | ||
} | ||
if (SelectSMRDOffset(N0, SOffset, Offset, Imm32Only, IsBuffer)) { | ||
if (SelectSMRDOffset(N0, SOffset, Offset, Imm32Only, IsBuffer, HasSOffset, | ||
ImmOffset)) { | ||
SBase = N1; | ||
return true; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The offset is already passed in, having a second copy of it in the immediate case is harder to follow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to get the ImmOffset value? When selecting for SOffset, the Offset pointer is a nullptr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just handle that case as ImmOffset=0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I mean in the case where it is attempting to select both an SOffset and Offset (Immediate). It will first attempt to select the immediate Offset then the SOffset. When selecting for SOffset, the Offset pointer is not passed in so when selecting SOffset, it has no knowledge of the immediate Offset value.
nullptr is passed for Offset here: https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp#L2082