This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] implemented assembler pseudo instructions for RV32I and RV64I
ClosedPublic

Authored by niosHD on Dec 6 2017, 8:19 AM.

Details

Summary

Adds the assembler pseudo instructions of RV32I and RV64I which can
be mapped to a single canonical instruction. The missing pseudo
instructions (e.g., call, tail, ...) are marked as TODO. Other
things, like for example PCREL_LO, have to be implemented first.

Currently, alias emission is disabled by default to keep the patch
minimal. Alias emission by default will be enabled in a subsequent
patch which also updates all affected tests. Note that this patch
should actually break the floating point MC tests. However, the
used FileCheck configuration is not tight enought to detect the
breakage.

Diff Detail

Repository
rL LLVM

Event Timeline

niosHD created this revision.Dec 6 2017, 8:19 AM
asb edited edge metadata.Dec 7 2017, 3:19 AM

Thanks Mario, this is really helpful. From a first look this looks good. With the RV32F+RV32D MC layer patches now reviewed+merged, I've gone ahead and committed the fairly straight-forward patches for RV64* MC layer support. Could you please rebase and enable the RV64I mnemonics, then I can do a more detailed review before committing?

niosHD updated this revision to Diff 125952.Dec 7 2017, 7:16 AM
niosHD edited the summary of this revision. (Show Details)

Rebased as requested.

Glad you like it Alex. I am looking forward to your comments.

asb added a comment.Dec 8 2017, 4:39 AM

Just a few minor niggles in the inline comments. Thanks for matching the table in the ISA manual so directly - it made this trivial to review.

See my inline comments about enabling this by default - I'm pretty open to doing this however you prefer, so it's up to you.

Are you planning to submit a follow-up patch for the floating point pseudoinstructions?

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
33 ↗(On Diff #125952)

Perhaps a name that better aligns with the -no-aliases flag used by the GNU tools would make sense? -riscv-no-aliases and NoAliases. We should probably emit aliases by default, matching the GNU behaviour. Of course that's a more disruptive changes, so as an alternative:

  • Update this patch to use riscv-aliases, disabled by default.
  • At a later point, I can switch it to a riscv-no-aliases argument that is false by default and update the test cases as necessary.

Only issue with the incremental approach is that it interferes with the current use of InstAlias for the rounding mode in floating point instructions. If it doesn't break the floating point MC layer tests (due to not matching to the end of the line), it probably should.

lib/Target/RISCV/RISCVInstrInfo.td
504 ↗(On Diff #125952)

For what it's worth I tend to indent these as if they were C++ namespaces (which LLVM coding style prefers not to indent). You'd probably want to add an extra newline before/after to separate these RV64-only aliases visually though.

525 ↗(On Diff #125952)

Apologies that this seems incredibly picky, but LLVM style prefers full sentences as comments - i.e. capitalised first letter, end with a full stop.

Thanks for spotting this behaviour. I'd recommend documenting that we do this to match GNU behaviour, e.g. "The GNU tools always emit the canonical mnemonic for the branch pseudoinstructions (e.g. bgt will be recognised by the assembler but never printed by objdump). Match this behaviour by setting a zero weight."

548 ↗(On Diff #125952)

Given that you've maintained the same ordering as the tables in the ISA manual everywhere else (thanks! That's made is really easy to review), I think it would make sense to order these as rdinstret, rdcyle, rdtime to match table 20.3 in the spec. Same for the RV32-only rd*h variants.

Thank you for the review Alex. I will incorporate your comments in an updated patch today.

I definitely plan to submit a follow-up patch with the remaining floating point alias instructions once this one is finished.

lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
33 ↗(On Diff #125952)

Matching the GNU tools flag name definitely makes sense since it makes it easier for users to find it. I will therefore switch to your proposed -riscv-no-aliases flag with the NoAliases variable name.

Regarding default behaviour I also planed to match the GNU tools eventually. However, regarding the process I am not sure myself and would appreciate your opinion. If you don't think it adds too much noise to the patch to update all affected test cases then I can directly integrate it. Otherwise, I thought about flipping the default and updating the test cases in a separate patch.

Interestingly, I don't get a test failure in the floating point MC tests due to disabling the printing of the aliases by default with this patch. I need to have a closer look why this is the case.

lib/Target/RISCV/RISCVInstrInfo.td
504 ↗(On Diff #125952)

Sure, will be updated.

525 ↗(On Diff #125952)

No worries, since it is my first upstream patch I am grateful for your detailed comments. I simply was not aware of that convention and will update it.

Regarding behaviour you give me too much credit. I simply thought that emitting different branching instructions is not worth the complexity. However, I will add a sentence which documents that the GNU tools do the same.

548 ↗(On Diff #125952)

Sure, I will update the order. Thank you for noticing.

asb added inline comments.Dec 11 2017, 3:20 AM
lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp
33 ↗(On Diff #125952)

I think the reason the floating point MC tests don't break is that the FileCheck doesn't match to the end of the line, so the extra format argument is just ignored. Adding --match-full-lines to the FileCheck invocation won't work, but presumably a regex can be written to check for end-of-line or space and tighten the checks up.

I think the best way of managing the switch is probably:

  • This patch leaves alias emission disabled by default. Note in the patch summary that it really should break the floating point MC tests but doesn't (and this will be addressed in a follow-up patch).
  • A follow-up patch enables alias emissions, updates the tests, and ideally tightens up the floating point MC layer tests

That way if there are unexpected issues it's easy to revert the patch that enables aliases by default.

niosHD updated this revision to Diff 126370.Dec 11 2017, 8:15 AM
niosHD edited the summary of this revision. (Show Details)

Addressed all comments for this patch. I already used riscv-no-aliases as command line flag but defaulted it to true instead of adding riscv-aliases which would be removed soon anyway.

asb accepted this revision.Dec 12 2017, 7:44 AM

Thanks, this is looking great!

This revision is now accepted and ready to land.Dec 12 2017, 7:44 AM
This revision was automatically updated to reflect the committed changes.