This is an archive of the discontinued LLVM Phabricator instance.

[ABI] Rewrite RegisterIsCalleeSaved
ClosedPublic

Authored by davide on Sep 2 2017, 9:13 PM.

Details

Summary

The goal of this patch is twofold:
First, it removes a wrong comment (at least, not correctly describing what the function does).
Then, it rewrites the function to use a stringswitch where the registers are enumerated explicitly instead of being computed programmatically. Other than being much shorter, it's much easier to read (and given the ABI won't change anytime soon, I don't think there's need to generalize).
While here, I added an assert that the register name is always empty, as the previous implementation of the function assumed so.

Do we want to add unittests for this to make sure the thing don't regress? I think it's a good thing to to anyway, but I wanted to reach consensus (and probably can be done in a follow-up commit).

Diff Detail

Repository
rL LLVM

Event Timeline

davide created this revision.Sep 2 2017, 9:13 PM
davide added inline comments.
source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
1916 ↗(On Diff #113675)

Currently this uses two cases as the underlying StringSwitch implementation allows up to 10 arguments. I have plans to switch it to use variadic templates so that this code can be rewritten to use a single case.

compnerd accepted this revision.Sep 3 2017, 1:41 PM

Nice cleanup :-)

source/Plugins/ABI/SysV-x86_64/ABISysV_x86_64.cpp
1918 ↗(On Diff #113675)

Would be nice to reoder these to rbp, ebp, rbx, ebx. It is easier to see that you are looking at the 32-bit and 64-bit register names that way due to the association.

This revision is now accepted and ready to land.Sep 3 2017, 1:41 PM
This revision was automatically updated to reflect the committed changes.