This is an archive of the discontinued LLVM Phabricator instance.

[VE] Add alternative names to registers
ClosedPublic

Authored by kaz7 on Apr 14 2020, 5:52 PM.

Details

Summary

VE uses identical names "%s0-63" to all generic registers. Change to use
alternative name mechanism among all generic registers instead of hard-
coding them.

Event Timeline

kaz7 created this revision.Apr 14 2020, 5:52 PM
kaz7 added a comment.Apr 14 2020, 5:55 PM

This time, I am using new feature so I would like to add reviewers who know about it.

This time, I am using new feature so I would like to add reviewers who know about it.

Makes sense.

One thing i noted, which should not go into this patch and is more of a cleanup thing: we probably want register name aliases also on the c++ side (eg something like VE::SP to refer to the stack pointer instead of using its SX register number).

kaz7 updated this revision to Diff 257909.Apr 15 2020, 5:14 PM

Modify include path to try to avoid clang-tidy error.

Ping @arsenm . Do you have any concerns with the use of altNames here?

arsenm added inline comments.Apr 20 2020, 7:28 AM
llvm/lib/Target/VE/MCTargetDesc/VEInstPrinter.cpp
39–41

Typo regitster

llvm/lib/Target/VE/VERegisterInfo.td
39

You can just lower case the original names unless you want to handle printing both capital and upper case for some reason

kaz7 updated this revision to Diff 258880.Apr 20 2020, 6:10 PM

Update code to follow @arsenm comments.

kaz7 marked 2 inline comments as done.Apr 20 2020, 6:12 PM
llvm/lib/Target/VE/MCTargetDesc/VEInstPrinter.cpp
39–41

Thank you. Corrected it.

llvm/lib/Target/VE/VERegisterInfo.td
39

Thanks. Changed to use lower case in string representations.

arsenm added inline comments.Apr 20 2020, 6:35 PM
llvm/lib/Target/VE/MCTargetDesc/VEInstPrinter.cpp
41

You should be able to directly use the output of getRegisterName without the lower now?

kaz7 marked an inline comment as done.Apr 20 2020, 8:28 PM
kaz7 added inline comments.
llvm/lib/Target/VE/MCTargetDesc/VEInstPrinter.cpp
41

That's true. I haven't noticed it. Thank you for very useful comments to reduce computation time. I really appreciate it.

kaz7 updated this revision to Diff 258892.Apr 20 2020, 8:29 PM

Remove lower() by lowering register name defined in VERegisterInfo.td.
Following an @arsenm suggesntion. Thanks!

arsenm accepted this revision.Apr 21 2020, 10:40 AM
arsenm added inline comments.
llvm/lib/Target/VE/MCTargetDesc/VEInstPrinter.cpp
41

You don't need to construct a stringref either

This revision is now accepted and ready to land.Apr 21 2020, 10:40 AM
kaz7 updated this revision to Diff 259142.Apr 21 2020, 6:23 PM

Removed StringRef constructor by following arsenm suggestion. Thanks!
Rebased to the latest master.

This revision was automatically updated to reflect the committed changes.