This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Support named operands for CSR instructions.
ClosedPublic

Authored by apazos on May 11 2018, 10:06 AM.

Diff Detail

Event Timeline

apazos created this revision.May 11 2018, 10:06 AM
asb added a comment.May 17 2018, 7:46 AM

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.

asb added a comment.EditedMay 23 2018, 2:44 PM

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.

asb added a subscriber: nhaehnle.Jun 21 2018, 8:19 AM

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

I am looking at this now, but it seems Nicolai's patch has not been merged yet.

apazos updated this revision to Diff 159824.Aug 8 2018, 4:25 PM

Updated implementation
Updated a few tests, more to be added.

apazos retitled this revision from WIP [RISCV] Supporting aliases for Machine level CSRs. to [RISCV] Support named operands for CSR instructions..Aug 8 2018, 4:28 PM
apazos updated this revision to Diff 160961.Aug 15 2018, 6:12 PM

Just rebased.

asb added a comment.Aug 16 2018, 3:13 AM

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).

nhaehnle added inline comments.Aug 23 2018, 12:42 AM
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.

apazos updated this revision to Diff 163250.Aug 29 2018, 6:45 PM
  • 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.

apazos updated this revision to Diff 163460.Aug 30 2018, 8:15 PM

Rebased patch, reworked test cases to keep all tests in test/MC/RISCV

asb added inline comments.Sep 6 2018, 8:13 AM
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.

apazos updated this revision to Diff 166037.Sep 18 2018, 2:35 PM

Rebased.

apazos marked an inline comment as done.Sep 20 2018, 10:29 AM
apazos added inline comments.
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

asb accepted this revision.Oct 4 2018, 4:48 AM

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.

This revision is now accepted and ready to land.Oct 4 2018, 4:48 AM
apazos marked an inline comment as done.Oct 4 2018, 10:58 AM

Sure, will do the cleanup of uimm12.

apazos updated this revision to Diff 168355.Oct 4 2018, 12:57 PM

Removed uimm12 related code.

This revision was automatically updated to reflect the committed changes.