Details
Diff Detail
Event Timeline
It might be worth looking at what AArch64 does. rL274576 introduced the strategy that is still used, which involves a new tablegen emitter generating an efficiently searchable table.
Are the DwarfRegNum values used here 'official'?
To use that infrastructure we need to clarify whether register names are indeed valid in the CSR instructions:
csrrs t1, 768, zero
csrrs t1, mstatus, zero - also valid?
And if register names are valid in the pseudo instructions:
csrs t1, 768
csrs t1, mstatus - also valid?
It is not clear from the ISA manual, how can we confirm?
We would have to allow an instruction to have 2 possible formats when parsing, a register name or immediate value.
RISC-V assembly syntax isn't really described completely in any source, so we're kind of stuck following what gas does. There is some documentation here which myself and other community members have contributed to, but nothing about register identifiers for CSR instructions.
From what I can see (in RISC-V code in the wild and by testing gas), CSR register names can be accepted in any location where the uimm5 CSR number is accepted. So the following are equally acceptable:
csrrs t1, mstatus, x0 csrr t1, mstatus csrrs t1, 768, x0 csrr t1, 768
I agree that fundamentally there's a need to support parsing either a register name or an immediate for the CSR instruction operands (and to convert back when printing). We should accept any uimm5, but also accept recognised CSR names.
Hi Ana, if you're planning on updating it would probably be worth following the work on a replacement for SearchableTable:
http://lists.llvm.org/pipermail/llvm-dev/2018-June/123915.html
https://reviews.llvm.org/D48015
CC @nhaehnle
Thanks Ana, some initial thoughts:
- clang-format suggests some alternative formatting in RISCVAsmParser.cpp and RISCVInstPrinter.h. I'm not a fan of putting enum KindTy on a single line as it prefers, but otherwise these seem like sensible reformattings
- You need to add RISCVUtils to required_libraries in lib/Target/RISCV/AsmParser/LLVMBuild.txt. I get a link error otherwise
- Why use ALLCAPS for the canonical CSR spelling. Lowercase would better match GNU behaviour
- It would be worth adding a check that call instret or similar still works. It should do due to the way the custom parser is invoked, but it's worth having the test case I think.
- It looks like the removal of the original RISCVBaseInfo.h wasn't picked up in this patch
Are all the tests meant to pass in the current iteration of the patch? I'm getting some failures (rvi-aliases-valid.s and rv32i-aliases-valid.s).
| lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp.rej | ||
|---|---|---|
| 1 ↗ | (On Diff #160961) | This file is garbage. |
| lib/Target/RISCV/RISCVSystemOperands.td | ||
| 22–37 | The field names should correspond to the C++ names, so Requires should be FeaturesRequired. (This doesn't affect anything here because that field isn't used as an index, I just personally think it's good style.) I would also remove the fields that aren't actually used anywhere The TableGen patch has landed quite some time ago, so I'd suggest you change this to (untested, off the top of my head): class SysReg<string name, bits<12> op> {
string Name = name;
bits<12> Encoding = op;
// Privilege Access: Read and Write = 0, 1, 2; Read-Only = 3.
// Privilege Mode: User = 0, System = 1 or Machine = 3.
bits<2> ReadWrite = op{11-10};
bits<2> Mode = op{9-8};
// FIXME: check what bits 7-6 correspond to.
bits<2> Extra = op{7-6};
// Register number without the privilege bits.
bits<6> Number = op{5-0};
code FeaturesRequired = [{ {} }];
}
def SysRegsList : GenericTable {
let FilterClass = "SysReg";
let Fields = ["Name", "Encoding", "ReadWrite", "Mode", "Extra", "Number", "FeaturesRequired"];
let PrimaryKey = ["Encoding"];
let PrimaryKeyName = "lookupSysRegByEncoding";
}
def lookupSysRegByName : SearchIndex {
let Table = SysRegsList;
let Key = ["Name"];
}Finally, I'm not sure why you have those extra fields that just extract a part of the encoding. For example, I don't see Extra or ReadWrite being used anywhere. With the GenericTable, the C++ struct can be decoupled from the TableGen class. So you can keep the ReadWrite field in the TableGen SysReg class if you think it's valuable as documentation, but at the same time remove the ReadWrite member of the C++ SysReg class; you just need to remove "ReadWrite" from the list of Fields in the SysRegsList definition. That way, you get a more compact table at compiler runtime. | |
- I ran clang-format and incorporated the changes, but did not change the existing enums/switch declarations.
- Added missing RISCVUtils to required_libraries in lib/Target/RISCV/AsmParser/LLVMBuild.txt.
- Using lower case for CSR register names.
- Added a check that 'call instret, 'or any CSR register, still works properly.
- Removed the original RISCVBaseInfo.h
- Completed the tests I wanted to add. All tests are passing in my local build.
- Updated TableGen implementation. I left FIXME comments for the unused fields for now. In a future patch (read CSR builtins) we will need them.
To discuss:
- Behavior of printing CSR name or number depending on required feature being present.
--When assembling
---If you pass an immediate, accept the immediate, that might correspond or not to a CSR register name, if the immediate value is valid, regardless of the required features.
---If you pass a valid CSR register name, check the required features, and if not present, print error message.
--When disassembling
---If the required features are present, print the CSR register name
---If not, print the CSR number.
- I wanted compatibility with GNU AS, but GNU AS has some issues, e.g., it assembles instructions with CSR register names that do not exist in 64bit.
So we are not totally compatible yet.
- There is not convenient feature for checking 32 bit, so I added a isRV32Only type of field
Thanks, the TableGen part looks good to me. I can't speak for the actual RISC-V changes.
| test/MC/RISCV/function-call-invalid.s | ||
|---|---|---|
| 13 ↗ | (On Diff #163460) | I think this test addition and comment are unnecessary? The rejection of call with an immediate operand is a bug vs the GNU assembler, and once accepted there shouldn't be an issue in calling an absolute address that happens to coincide wit ha CSR name. I think this is just a test line that should be deleted, as I couldn't see any logic that tries specifically to invalidate calls to immediates that overlap with CSR values. |
| test/MC/RISCV/function-call-invalid.s | ||
|---|---|---|
| 13 ↗ | (On Diff #163460) | Done. I left only the test that checks call with function name matching CSR name |
This looks good to me, thank you.
I believe def uimm12 : Operand<XLenVT> {... can be dropped after this change as it was only used in the CSR instructions. Then we can drop the references to it from RISCVAsmParser. It would be great if you could do that while committing this patch.
Having to introduce an isRV32Only is a shame, but I think it's the best available solution given the fairly low-level handling of feature checks with the GenericTable infrastructure.
The field names should correspond to the C++ names, so Requires should be FeaturesRequired. (This doesn't affect anything here because that field isn't used as an index, I just personally think it's good style.)
I would also remove the fields that aren't actually used anywhere
The TableGen patch has landed quite some time ago, so I'd suggest you change this to (untested, off the top of my head):
class SysReg<string name, bits<12> op> { string Name = name; bits<12> Encoding = op; // Privilege Access: Read and Write = 0, 1, 2; Read-Only = 3. // Privilege Mode: User = 0, System = 1 or Machine = 3. bits<2> ReadWrite = op{11-10}; bits<2> Mode = op{9-8}; // FIXME: check what bits 7-6 correspond to. bits<2> Extra = op{7-6}; // Register number without the privilege bits. bits<6> Number = op{5-0}; code FeaturesRequired = [{ {} }]; } def SysRegsList : GenericTable { let FilterClass = "SysReg"; let Fields = ["Name", "Encoding", "ReadWrite", "Mode", "Extra", "Number", "FeaturesRequired"]; let PrimaryKey = ["Encoding"]; let PrimaryKeyName = "lookupSysRegByEncoding"; } def lookupSysRegByName : SearchIndex { let Table = SysRegsList; let Key = ["Name"]; }Finally, I'm not sure why you have those extra fields that just extract a part of the encoding. For example, I don't see Extra or ReadWrite being used anywhere.
With the GenericTable, the C++ struct can be decoupled from the TableGen class. So you can keep the ReadWrite field in the TableGen SysReg class if you think it's valuable as documentation, but at the same time remove the ReadWrite member of the C++ SysReg class; you just need to remove "ReadWrite" from the list of Fields in the SysRegsList definition. That way, you get a more compact table at compiler runtime.