[AMDGPU] Fix wrong operand value when floating-point value is used as operand of type i16 #84106
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
You can test this locally with the following command:git-clang-format --diff 3f7aa042b657671319f994ad3fb7c3eb79a6fe00 57a134c493f8cb2afe1e31a7a4fa4270463706f5 -- llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cppView the diff from clang-format here.diff --git a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
index 5050aec261..fd8ce9e07f 100644
--- a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -2065,8 +2065,8 @@ bool AMDGPUOperand::isInlinableImm(MVT type) const {
break;
}
return isInlineableLiteralOp16(
- static_cast<uint16_t>(FPLiteral.bitcastToAPInt().getZExtValue()), type,
- AsmParser->hasInv2PiInlineImm());
+ static_cast<uint16_t>(FPLiteral.bitcastToAPInt().getZExtValue()),
+ type, AsmParser->hasInv2PiInlineImm());
}
// Check if single precision literal is inlinable
|
shiltian
force-pushed
the
fp-i16
branch
3 times, most recently
from
70996de
to
e48c168
Compare
… operand of type i16 Based on the section "OPF_INV2PI_16" of the spec, when a floating-point value is used as operand of type `i16`, the 32-bit representation of the constant truncated to the 16 LSBs should be used. Currently we directly use the FP16 representation, which doesn't conform with the spec. For example, when `0.5` is used, for now we take it as `0x3800` because that is the encoding of `<half 0.5>`. Instead, it should be `0x3f000000` truncated to 16 LSB, which is `0x0000`.
|
Is there any script to update those tests? It is quite a nightmare to update them manually. |
arsenm
reviewed
Mar 6, 2024
| @@ -2045,9 +2047,26 @@ bool AMDGPUOperand::isInlinableImm(MVT type) const { | |||
| return false; | |||
|
|
|||
| if (type.getScalarSizeInBits() == 16) { | |||
There was a problem hiding this comment.
This is redundant with the switch over the handled 16-bit types
jayfoad
reviewed
Mar 6, 2024
Comment on lines
11191
to
+11195
| v_cvt_f16_u16_e32 v5, 0.5 | ||
| // GFX10: encoding: [0xff,0xa0,0x0a,0x7e,0x00,0x38,0x00,0x00] | ||
| // GFX10: encoding: [0x80,0xa0,0x0a,0x7e] | ||
|
|
||
| v_cvt_f16_u16_e32 v5, -4.0 | ||
| // GFX10: encoding: [0xff,0xa0,0x0a,0x7e,0x00,0xc4,0x00,0x00] | ||
| // GFX10: encoding: [0x80,0xa0,0x0a,0x7e] |
There was a problem hiding this comment.
Note these two instructions now assemble to identical binary since the low 16 bits of f32 0.5 and f32 -4.0 are identical. Can you add tests with a more interesting literal, like inv2pi, which has non-0 low 16 bits?
|
This patch will be merged into 530f0e6 when it is relanded. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Based on the section "OPF_INV2PI_16" of the spec, when a floating-point value is
used as operand of type
i16, the 32-bit representation of the constanttruncated to the 16 LSBs should be used. Currently we directly use the FP16
representation, which doesn't conform with the spec.
For example, when
0.5is used, for now we take it as0x3800because that isthe encoding of
<half 0.5>. Instead, it should be0x3f000000truncated to 16LSB, which is
0x0000.