[ARM][MC] Add GNU Alias for ldrexd and strexd instructions #86507
Conversation
These aliasses were supported previously in llvm-mc but support was lost at some point and for a while it seems to have caused a crash. This adds back the alternate forms and tidies up this section of code a little.
|
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-arm Author: Alfie Richards (AlfieRichardsArm) ChangesThese aliases were supported previously in llvm-mc but support was lost at some point and for a while it seems to have caused a crash. This adds back the alternate forms and tidies up this section of code a little. See #83436 (comment) for the initial report regarding this change. Full diff: https://github.com/llvm/llvm-project/pull/86507.diff 2 Files Affected:
diff --git a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
index 2ad576ab1a9caa..b50698996e62a5 100644
--- a/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
+++ b/llvm/lib/Target/ARM/AsmParser/ARMAsmParser.cpp
@@ -7343,34 +7343,46 @@ bool ARMAsmParser::ParseInstruction(ParseInstructionInfo &Info, StringRef Name,
// automatically
// expressed as a GPRPair, so we have to manually merge them.
// FIXME: We would really like to be able to tablegen'erate this.
- if (!isThumb() && Operands.size() > MnemonicOpsEndInd + 1 &&
+ bool IsLoad = (Mnemonic == "ldrexd" || Mnemonic == "ldaexd");
+ if (!isThumb() && Operands.size() > MnemonicOpsEndInd + 1 + (!IsLoad) &&
(Mnemonic == "ldrexd" || Mnemonic == "strexd" || Mnemonic == "ldaexd" ||
Mnemonic == "stlexd")) {
- bool isLoad = (Mnemonic == "ldrexd" || Mnemonic == "ldaexd");
- unsigned Idx = isLoad ? MnemonicOpsEndInd : MnemonicOpsEndInd + 1;
+ unsigned Idx = IsLoad ? MnemonicOpsEndInd : MnemonicOpsEndInd + 1;
ARMOperand &Op1 = static_cast<ARMOperand &>(*Operands[Idx]);
ARMOperand &Op2 = static_cast<ARMOperand &>(*Operands[Idx + 1]);
const MCRegisterClass &MRC = MRI->getRegClass(ARM::GPRRegClassID);
- // Adjust only if Op1 and Op2 are GPRs.
- if (Op1.isReg() && Op2.isReg() && MRC.contains(Op1.getReg()) &&
- MRC.contains(Op2.getReg())) {
+ bool IsGNUAlias = !(Op2.isReg() && MRC.contains(Op2.getReg()));
+ // Adjust only if Op1 is a GPR.
+ if (Op1.isReg() && MRC.contains(Op1.getReg())) {
unsigned Reg1 = Op1.getReg();
- unsigned Reg2 = Op2.getReg();
unsigned Rt = MRI->getEncodingValue(Reg1);
- unsigned Rt2 = MRI->getEncodingValue(Reg2);
- // Rt2 must be Rt + 1 and Rt must be even.
- if (Rt + 1 != Rt2 || (Rt & 1)) {
- return Error(Op2.getStartLoc(),
- isLoad ? "destination operands must be sequential"
- : "source operands must be sequential");
+ // Check we are not in the GNU alias case with only one of the register
+ // pair specified
+ if (!IsGNUAlias) {
+ unsigned Reg2 = Op2.getReg();
+ unsigned Rt2 = MRI->getEncodingValue(Reg2);
+ // Rt2 must be Rt + 1.
+ if (Rt + 1 != Rt2)
+ return Error(Op2.getStartLoc(),
+ IsLoad ? "destination operands must be sequential"
+ : "source operands must be sequential");
}
+ // Rt bust be even
+ if (Rt & 1)
+ return Error(
+ Op1.getStartLoc(),
+ IsLoad ? "destination operands must start start at an even register"
+ : "source operands must start start at an even register");
+
unsigned NewReg = MRI->getMatchingSuperReg(
Reg1, ARM::gsub_0, &(MRI->getRegClass(ARM::GPRPairRegClassID)));
Operands[Idx] =
ARMOperand::CreateReg(NewReg, Op1.getStartLoc(), Op2.getEndLoc());
- Operands.erase(Operands.begin() + Idx + 1);
+ // Only remove redundent operand if not in GNU alias case
+ if (!IsGNUAlias)
+ Operands.erase(Operands.begin() + Idx + 1);
}
}
diff --git a/llvm/test/MC/ARM/basic-arm-instructions.s b/llvm/test/MC/ARM/basic-arm-instructions.s
index 84a7cf52fa30e0..9f3a5cd4afa792 100644
--- a/llvm/test/MC/ARM/basic-arm-instructions.s
+++ b/llvm/test/MC/ARM/basic-arm-instructions.s
@@ -1202,6 +1202,10 @@ Lforward:
@ CHECK: ldrex r1, [r7] @ encoding: [0x9f,0x1f,0x97,0xe1]
@ CHECK: ldrexd r6, r7, [r8] @ encoding: [0x9f,0x6f,0xb8,0xe1]
+@ GNU alias
+ ldrexd r6, [r8]
+@ CHECK: ldrexd r6, r7, [r8] @ encoding: [0x9f,0x6f,0xb8,0xe1]
+
@------------------------------------------------------------------------------
@ LDRHT
@------------------------------------------------------------------------------
@@ -2904,6 +2908,10 @@ Lforward:
@ CHECK: strex r2, r1, [r7] @ encoding: [0x91,0x2f,0x87,0xe1]
@ CHECK: strexd r6, r2, r3, [r8] @ encoding: [0x92,0x6f,0xa8,0xe1]
+@ GNU alias
+ strexd r6, r2, [r8]
+@ CHECK: strexd r6, r2, r3, [r8] @ encoding: [0x92,0x6f,0xa8,0xe1]
+
@------------------------------------------------------------------------------
@ STR
@------------------------------------------------------------------------------
|
|
✅ With the latest revision this PR passed the Python code formatter. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
I can confirm that when I try to build our internal code base that was failing earlier with these changes it now successfully builds again.
I have also looked over your change and it generally looks fine to me, although I would prefer that someone more familiar with this area actually approves it before committing the change.
Regarding testing, I notice that this change also seems to affect ldaexd and probably stlexd. Are there tests for these instructions in this case or more generally? Also I'm not sure if we generally do negative tests for these things, but if we do, can we add negative tests for destination/source operands not being sequential and not starting at an even register?
Alfie, do you think it's worth adding extra test cases here? Or would they provide limited benefit? |
|
AlfieRichardsArm
force-pushed
the
refactor-strexd-and-ldrexd
branch
from
c28af19
to
d5ab531
Compare
|
I've cleaned this up a bit and clarified the |
These aliases were supported previously in llvm-mc but support was lost at some point and for a while it seems to have caused a crash.
This adds back the alternate forms and tidies up this section of code a little.
See #83436 (comment) for the initial report regarding this change.