Page MenuHomePhabricator

[COFF, ARM64] Add __getReg intrinsic
ClosedPublic

Authored by mgrang on Oct 3 2018, 11:18 AM.

Diff Detail

Repository
rC Clang

Event Timeline

mgrang created this revision.Oct 3 2018, 11:18 AM
mgrang added a comment.EditedOct 3 2018, 11:24 AM

I have some questions about the behavior of getReg:

__getReg ultimately invokes getRegisterByName in http://llvm.org/doxygen/AArch64ISelLowering_8cpp_source.html.

You can __getReg only those registers which appear in the switch-case inside that function. Even then the logic there checks if the requested register is reserved. If not, then it makes the reg = 0 which results in "Invalid register name" error.

I am not sure how this should be dealt with. Is it fine to add all regs (0-31) to the switch-case in getRegisterByName? And should the checks for isRegReserved be bypassed for COFF?

I have some questions about the behavior of getReg:

__getReg ultimately invokes getRegisterByName in http://llvm.org/doxygen/AArch64ISelLowering_8cpp_source.html.

You can __getReg only those registers which appear in the switch-case inside that function. Even then the logic there checks if the requested register is reserved. If not, then it makes the reg = 0 which results in "Invalid register name" error.

I am not sure how this should be dealt with. Is it fine to add all regs (0-31) to the switch-case in getRegisterByName? And should the checks for isRegReserved be bypassed for COFF?

I don't really know about this one, I'll add a few more people who might be able to comment on it. @efriedma @peter.smith

read_register only allows reading reserved registers because reading an allocatable register is meaningless (the compiler can store arbitrary data in allocatable registers).

The same applies to __getReg; given that, I would assume real code doesn't use anything other than __getReg(18) or __getReg(31), so we can just reject any other constant.

lib/CodeGen/CGBuiltin.cpp
6583

Semantic checks belong in SemaChecking, not here. IIRC you can request constant checking in the .def file, although I don't remember the syntax off the top of my head.

6592

There is no register named x31. Maybe you need a special case here to use "sp" if the input is 31?

mgrang updated this revision to Diff 168192.Oct 3 2018, 4:04 PM
mgrang marked 2 inline comments as done.
efriedma added inline comments.Oct 4 2018, 12:33 PM
lib/CodeGen/CGBuiltin.cpp
6589

Could you just write this as std::string Reg = Value == 31 ? "sp" : "x" + Value.toString(10);, and get rid of StrVal?

mgrang updated this revision to Diff 168353.Oct 4 2018, 12:50 PM
mgrang marked an inline comment as done.

Needs a testcase for the error message like we have for other intrinsics in test/Sema/builtins-arm64.c . Otherwise LGTM.

mgrang updated this revision to Diff 168368.Oct 4 2018, 1:54 PM
mgrang updated this revision to Diff 168371.Oct 4 2018, 1:56 PM
efriedma accepted this revision.Oct 4 2018, 2:08 PM

LGTM

This revision is now accepted and ready to land.Oct 4 2018, 2:08 PM
This revision was automatically updated to reflect the committed changes.