This is an archive of the discontinued LLVM Phabricator instance.

TLI: Remove DAG argument from getRegisterByName
ClosedPublic

Authored by arsenm on Sep 15 2019, 7:25 PM.

Details

Summary

Replace with the MachineFunction. X86 is the only user, and only uses
it for the function. This removes one obstacle from using this in
GlobalISel. The other is the more tolerable EVT argument.

The X86 use of the function seems questionable to me. It checks hasFP,
before frame lowering.

Diff Detail

Event Timeline

arsenm created this revision.Sep 15 2019, 7:25 PM

For context, read_register was introduced to allow use of the "global named register" GNU extension.

GNU's version allows the programmer to read any register, on both global and local contexts, which is nuts. After a long discussion, we decided to only allow global read of the stack pointer, which was the only meaningful use of this feature.

The kernel community agreed and changed their code (in a lot of target-specific files) to remove the dangerous use of reading registers directly without an inline asm context.

With that, Clang's implementation ended up as "only SP", which then unfortunately, it seems, allowed some assumptions to shape the code.

I don't know x86 well enough, but this code seems to be checking for some relationship between the frame and stack pointers that don't exist in other targets.

Regarding the change, it looks good to me, but I'll let other people chime in, as it's been a long time since I looked at that code.

cheers,
--renato

The code is attempting to ensure that you don't use RBP as a global register unless RBP is actually a reserved register (being used as the frame pointer in the function.) That's a reasonable goal, but maybe there's another better way to be doing it.

I think this function probably ought to be renamed. It's really not clear from its name that it's intended for such a narrow purpose -- _only_ for implementing the read_register/write_register intrinsics.

I think this function probably ought to be renamed. It's really not clear from its name that it's intended for such a narrow purpose -- _only_ for implementing the read_register/write_register intrinsics.

Agreed.

The "original" purpose was to work for all registers, but the read_/write_register scope shrunk a lot, and it's the only user.

If we want a generic name->register match, then we want that to be generated by table-gen. But then we still need to restrict which ones are useful, per target, for read/write.

Since we don't really need the former, I agree a rename would make more sense.

bogner accepted this revision.Sep 18 2019, 10:43 AM

Regardless of what we want to do with getRegisterByName (ie, rename it to something more targeted, generalize it, etc) this change on its own is a strict improvement. LGTM.

This revision is now accepted and ready to land.Sep 18 2019, 10:43 AM
arsenm closed this revision.Sep 30 2019, 6:44 PM

r373292