-
Notifications
You must be signed in to change notification settings - Fork 14.8k
[Hexagon] Add missing operand when disassembling Y4_crswap10 #153849
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
Conversation
Auto-generated decoder fails to add the $sgp10 operand because it has no encoding bits. Provide a custom decoder function that adds the operand. Fixes llvm#153829.
@llvm/pr-subscribers-mc @llvm/pr-subscribers-backend-hexagon Author: Sergei Barannikov (s-barannikov) ChangesAuto-generated decoder fails to add the $sgp10 operand because it has no encoding bits. Fixes #153829. Full diff: https://github.com/llvm/llvm-project/pull/153849.diff 4 Files Affected:
diff --git a/llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp b/llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp
index 22cff7c80fa01..fcda20dc78d15 100644
--- a/llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp
+++ b/llvm/lib/Target/Hexagon/Disassembler/HexagonDisassembler.cpp
@@ -172,6 +172,11 @@ static DecodeStatus s32_0ImmDecoder(MCInst &MI, unsigned tmp,
const MCDisassembler *Decoder);
static DecodeStatus brtargetDecoder(MCInst &MI, unsigned tmp, uint64_t Address,
const MCDisassembler *Decoder);
+
+static DecodeStatus decodeCRSWAP10(MCInst &Inst, unsigned Bits,
+ uint64_t Address,
+ const MCDisassembler *Decoder);
+
#include "HexagonDepDecoders.inc"
#include "HexagonGenDisassemblerTables.inc"
@@ -841,6 +846,16 @@ static DecodeStatus brtargetDecoder(MCInst &MI, unsigned tmp, uint64_t Address,
return MCDisassembler::Success;
}
+static DecodeStatus decodeCRSWAP10(MCInst &Inst, unsigned int Bits,
+ uint64_t Address,
+ const MCDisassembler *Decoder) {
+ unsigned RegNo = fieldFromInstruction(Bits, 16, 5);
+ DecodeDoubleRegsRegisterClass(Inst, RegNo, Address, Decoder);
+ DecodeDoubleRegsRegisterClass(Inst, RegNo, Address, Decoder);
+ Inst.addOperand(MCOperand::createReg(Hexagon::SGP1_0));
+ return MCDisassembler::Success;
+}
+
static const uint16_t SysRegDecoderTable[] = {
Hexagon::SGP0, Hexagon::SGP1, Hexagon::STID,
Hexagon::ELR, Hexagon::BADVA0, Hexagon::BADVA1,
diff --git a/llvm/lib/Target/Hexagon/HexagonDepInstrFormats.td b/llvm/lib/Target/Hexagon/HexagonDepInstrFormats.td
index 75e87c95f2c48..661b948346126 100644
--- a/llvm/lib/Target/Hexagon/HexagonDepInstrFormats.td
+++ b/llvm/lib/Target/Hexagon/HexagonDepInstrFormats.td
@@ -3049,7 +3049,6 @@ class Enc_cf1927 : OpcodeHexagon {
class Enc_d0fe02 : OpcodeHexagon {
bits <5> Rxx32;
let Inst{20-16} = Rxx32{4-0};
- bits <0> sgp10;
}
class Enc_d15d19 : OpcodeHexagon {
bits <1> Mu2;
diff --git a/llvm/lib/Target/Hexagon/HexagonDepInstrInfo.td b/llvm/lib/Target/Hexagon/HexagonDepInstrInfo.td
index ae96753f40cf2..993db0344e1fd 100644
--- a/llvm/lib/Target/Hexagon/HexagonDepInstrInfo.td
+++ b/llvm/lib/Target/Hexagon/HexagonDepInstrInfo.td
@@ -41176,6 +41176,8 @@ let Inst{31-21} = 0b01101101100;
let Uses = [SGP0, SGP1];
let Defs = [SGP0, SGP1];
let Constraints = "$Rxx32 = $Rxx32in";
+// $sgp10 operand has no encoding bits, we have to add the operand manually.
+let DecoderMethod = "decodeCRSWAP10";
}
def Y4_l2fetch : HInst<
(outs),
diff --git a/llvm/test/MC/Hexagon/system-inst.s b/llvm/test/MC/Hexagon/system-inst.s
index 7bc1533598532..07f7ca0acb2dc 100644
--- a/llvm/test/MC/Hexagon/system-inst.s
+++ b/llvm/test/MC/Hexagon/system-inst.s
@@ -89,6 +89,9 @@ crswap(r12,sgp0)
#CHECK: 652dc000 { crswap(r13,sgp1) }
crswap(r13,sgp1)
+#CHECK: 6d8ec000 { crswap(r15:14,s1:0) }
+crswap(r15:14,sgp1:0)
+
#CHECK: 660fc00e { r14 = getimask(r15) }
r14=getimask(r15)
|
I think it can be easier to fix this by defining double-size crswap the same way as single crswap. The difference is in the single-register crswap, sgp0/1 are hard-coded in the instruction syntax, while the double-register one uses a register reference ( |
I agree that would be better. However, it doesn't seem possible to put "sgp1:0" in the asm string -- it will get tokenized into three different tokens by TableGen backend when generating AsmMatcher. |
I mean, HexagonAsmParser parses "sgp1:0" as a register, but the generated AsmMatcher expects three AsmOperands for this Operand (Register SGP1, Token ":", and Immediate 0) because this is how TableGen backend parses the asm string. |
I see. |
Another option would be to introduce a register class consisting only of |
It looks like the operand is not created because there is no bitfield for it in the encoding. I don't know if there is a valid method in LLVM in general to create a fake operand without overriding the whole instruction. In Hexagon, similar cases are handled in a switch inside |
Thanks for the pointer, I'll do it there for consistency.
There isn't. Some targets override DecoderMethod, others go the Hexagon way of adding missing operands after decoding is complete. I'm working on fixing this (and this is how I found the bug). |
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.
Thank you!
5d1faac
to
5bdf272
Compare
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/154/builds/20299 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/66/builds/17974 Here is the relevant piece of the build log for the reference
|
/cherry-pick 76d993b |
/pull-request #153926 |
Auto-generated decoder fails to add the $sgp10 operand because it has no encoding bits.
Work around this by adding the missing operand after decoding is complete.
Fixes #153829.