Page MenuHomePhabricator

[RISCV] Add Option for Printing Architectural Register Names
ClosedPublic

Authored by lenary on Aug 8 2019, 6:46 AM.

Details

Summary

This is an option primarily to use during testing. Instead of always
printing registers using their ABI names, this allows a user to request they
are printed with their architectural name.

This is then used in the register constraint tests to ensure the mapping
between architectural and abi names is correct.

Diff Detail

Repository
rL LLVM

Event Timeline

lenary created this revision.Aug 8 2019, 6:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 6:46 AM
asb added inline comments.Aug 8 2019, 7:00 AM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
44 ↗(On Diff #214143)

The description makes it unclear that this affects FPRs as well. Maybe "architectural register names such as x0-x31 rather than the ABI name"?

lenary updated this revision to Diff 214147.Aug 8 2019, 7:11 AM

Address review feeback

  • Clarify flag name and description
lenary marked an inline comment as done.Aug 8 2019, 7:13 AM
apazos added inline comments.Aug 12 2019, 2:51 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
43 ↗(On Diff #214147)

Do we really need another flag? Or can we just reuse riscv-no-aliases?

I think with GNU toolchain, and when calling llvm-mc/llvm-objdump , -M no-aliases will avoid printing any aliases including architecture aliases. There is no separate flag for controlling printing arch reg names.

lenary marked an inline comment as done.Aug 13 2019, 3:22 AM
lenary added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
43 ↗(On Diff #214147)

I checked with my version of objdump (seems to be version 2.32) and it separates -M no-aliases and -M numeric (the latter of which prints the architectural register names). I'm happy to rename the flag, but the flags do seem to be separate.

I also thought a separate flag would be a less invasive change.

This flag definition becomes the definition for llvm-mc, and we don't yet support target-specific options using the -M flag for risc-v (but we should).

luismarques added inline comments.Aug 14 2019, 3:16 PM
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp
44 ↗(On Diff #214147)

Giving an example ("such as") and providing a range ("x0-x31", which tends to suggest it describes the range of possibilities) seems a bit of a weird mismatch. Some possible alternatives:

  • "such as x0"
  • "such as x0, f8, v3..."
  • "such as x2, instead of sp"?
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h
51 ↗(On Diff #214147)

Does this fall under the umbrella of the unsigned -> Register type transition? If so, please update the patch accordingly. (Apply the comment to the other cases in this patch).

lenary updated this revision to Diff 215378.Aug 15 2019, 5:59 AM
lenary marked 2 inline comments as done.

Address review feedback

  • Update flag description
lenary marked an inline comment as done.Aug 15 2019, 5:59 AM
lenary added inline comments.
llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.h
51 ↗(On Diff #214147)

Not yet, because the two-argument version is generated by tablegen.

asb accepted this revision.Aug 19 2019, 5:28 AM

LGTM, thanks!

This revision is now accepted and ready to land.Aug 19 2019, 5:28 AM
asb requested changes to this revision.Aug 19 2019, 5:33 AM

Actually changing my mind. This would be better tested by adopting something like arch-reg-names.s from D66139 (and extending that to handle FP regs), and leaving the inline-asm tests alone (huge increase in test size, which makes them harder to follow). Make that change, and then it looks good to me.

This revision now requires changes to proceed.Aug 19 2019, 5:33 AM
lenary updated this revision to Diff 219383.Sep 9 2019, 10:01 AM

Address Review Feedback:

  • Use numeric-reg-names*.s, based on arch-reg-names.s from D66139
  • Revert changes to inline-asm-*abi-names.ll
  • Rebase changes

Tests updated, they're now a more reasonable size. I'll need to update D66139.

asb accepted this revision.Sep 10 2019, 8:37 AM

LGTM, thanks!

This revision is now accepted and ready to land.Sep 10 2019, 8:37 AM
This revision was automatically updated to reflect the committed changes.