[AMDGPU][True16][MC] validate op_sel and .l/.h syntax #123250
Merged
Conversation
This file contains hidden or 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
broxigarchen
changed the title
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
broxigarchen
force-pushed
the
main-merge-true16-mc-print
branch
from
ad4918e to
34d51b7
Compare
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-amdgpu Author: Brox Chen (broxigarchen) Changescheck if op_sel is consistent with .l/.h syntax if both are presented Full diff: https://github.com/llvm/llvm-project/pull/123250.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
index d8f441d1ccfe44..2316b97d80f281 100644
--- a/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
+++ b/llvm/lib/Target/AMDGPU/AsmParser/AMDGPUAsmParser.cpp
@@ -33,6 +33,7 @@
#include "llvm/MC/MCParser/MCAsmParser.h"
#include "llvm/MC/MCParser/MCParsedAsmOperand.h"
#include "llvm/MC/MCParser/MCTargetAsmParser.h"
+#include "llvm/MC/MCRegisterInfo.h"
#include "llvm/MC/MCSymbol.h"
#include "llvm/MC/TargetRegistry.h"
#include "llvm/Support/AMDGPUMetadata.h"
@@ -1536,6 +1537,10 @@ class AMDGPUAsmParser : public MCTargetAsmParser {
return getFeatureBits()[AMDGPU::FeatureFlatInstOffsets];
}
+ bool hasTrue16Insts() const {
+ return getFeatureBits()[AMDGPU::FeatureTrue16BitInsts];
+ }
+
bool hasArchitectedFlatScratch() const {
return getFeatureBits()[AMDGPU::FeatureArchitectedFlatScratch];
}
@@ -1777,6 +1782,7 @@ class AMDGPUAsmParser : public MCTargetAsmParser {
bool validateMIMGDim(const MCInst &Inst, const OperandVector &Operands);
bool validateMIMGMSAA(const MCInst &Inst);
bool validateOpSel(const MCInst &Inst);
+ bool validateTrue16OpSel(const MCInst &Inst);
bool validateNeg(const MCInst &Inst, int OpName);
bool validateDPP(const MCInst &Inst, const OperandVector &Operands);
bool validateVccOperand(MCRegister Reg) const;
@@ -4651,6 +4657,39 @@ bool AMDGPUAsmParser::validateOpSel(const MCInst &Inst) {
return true;
}
+bool AMDGPUAsmParser::validateTrue16OpSel(const MCInst &Inst) {
+ if (!hasTrue16Insts())
+ return true;
+ const MCRegisterInfo *MRI = getMRI();
+ const unsigned Opc = Inst.getOpcode();
+ int OpSelIdx = AMDGPU::getNamedOperandIdx(Opc, AMDGPU::OpName::op_sel);
+ if (OpSelIdx == -1)
+ return true;
+ unsigned OpSelOpValue = Inst.getOperand(OpSelIdx).getImm();
+ // If the value is 0 we could have a default OpSel Operand, so conservatively
+ // allow it.
+ if (OpSelOpValue == 0)
+ return true;
+ unsigned OpCount = 0;
+ for (int OpName : {AMDGPU::OpName::src0, AMDGPU::OpName::src1,
+ AMDGPU::OpName::src2, AMDGPU::OpName::vdst}) {
+ int OpIdx = AMDGPU::getNamedOperandIdx(Inst.getOpcode(), OpName);
+ if (OpIdx == -1)
+ continue;
+ const MCOperand &Op = Inst.getOperand(OpIdx);
+ if (Op.isReg() &&
+ MRI->getRegClass(AMDGPU::VGPR_16RegClassID).contains(Op.getReg())) {
+ bool VGPRSuffixIsHi = AMDGPU::isHi16Reg(Op.getReg(), *MRI);
+ bool OpSelOpIsHi = ((OpSelOpValue & (1 << OpCount)) != 0);
+ if (OpSelOpIsHi != VGPRSuffixIsHi)
+ return false;
+ }
+ ++OpCount;
+ }
+
+ return true;
+}
+
bool AMDGPUAsmParser::validateNeg(const MCInst &Inst, int OpName) {
assert(OpName == AMDGPU::OpName::neg_lo || OpName == AMDGPU::OpName::neg_hi);
@@ -5132,6 +5171,11 @@ bool AMDGPUAsmParser::validateInstruction(const MCInst &Inst,
Error(getRegLoc(LDS_DIRECT, Operands), *ErrMsg);
return false;
}
+ if (!validateTrue16OpSel(Inst)) {
+ Error(getImmLoc(AMDGPUOperand::ImmTyOpSel, Operands),
+ "op_sel operand conflicts with 16-bit operand suffix");
+ return false;
+ }
if (!validateSOPLiteral(Inst)) {
Error(getLitLoc(Operands),
"only one unique literal operand is allowed");
|
arsenm
reviewed
Jan 22, 2025
Comment on lines
+5175
to
+5176
| Error(getImmLoc(AMDGPUOperand::ImmTyOpSel, Operands), | ||
| "op_sel operand conflicts with 16-bit operand suffix"); |
There was a problem hiding this comment.
added a test file
broxigarchen
force-pushed
the
main-merge-true16-mc-print
branch
from
34d51b7 to
fcbab85
Compare
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/15972 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/185/builds/12419 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/137/builds/12592 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/175/builds/12446 Here is the relevant piece of the build log for the reference |
kazutakahirata
added a commit
that referenced
this pull request
Jan 30, 2025
|
I've reverted this PR due to the buildbot failures above. |
Thanks! I'll take a look at it |
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Jan 30, 2025
…tax (#123250)" This reverts commit fabe747. Multiple buildbots are failing. See: llvm/llvm-project#123250
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/17583 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/18446 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/16/builds/13015 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/33/builds/10596 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/21494 Here is the relevant piece of the build log for the reference |
broxigarchen
added a commit
that referenced
this pull request
Feb 5, 2025
github-actions bot
pushed a commit
to arm/arm-toolchain
that referenced
this pull request
Feb 5, 2025
…5872) check if op_sel is consistent with .l/.h syntax if both are presented reopen this llvm/llvm-project#123250 since problem is resolved in llvm/llvm-project#125561
Icohedron
pushed a commit
to Icohedron/llvm-project
that referenced
this pull request
Feb 11, 2025
check if op_sel is consistent with .l/.h syntax if both are presented reopen this llvm#123250 since problem is resolved in llvm#125561
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.
check if op_sel is consistent with .l/.h syntax if both are presented