[PATCH] D22045: [X86] Support of no_caller_saved_registers attribute (Clang part)

Erich Keane via cfe-commits cfe-commits at lists.llvm.org
Thu Aug 4 10:40:35 PDT 2016


erichkeane added a comment.

Response on CGFucntionInfo.


================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:479
@@ +478,3 @@
+  /// Whether this function saved caller registers.
+  unsigned NoCallerSavedRegs : 1;
+
----------------
aaron.ballman wrote:
> This is unfortunate as it will bump the bit-field length to 33 bits, which seems rather wasteful. Are there any bits we can steal to bring this back down to a 32-bit bit-field?
I implemented this additional patch, but don't really know a TON about this area, so I have a few ideas but would like to get direction on it if possible.  I had the following ideas of where to get a few more bits:

CallingConvention/EffectiveCallingConvention/ASTCallingConvention:  This structure stores a pre-converted calling convention to the llvm::CallingConv enum (llvm/include/llvm/IR/CallingConv.h).  I'll note that the legal values for this go up to 1023 (so 8 bits isn't enough anyway!), though only up to 91 are currently used.  

HOWEVER, the clang CallingConv (include/clang/Basic/Specifiers.h :233) only has 16 items in it.  If we were instead to store THAT and convert upon access (a simple switch statement, already used constructing this value, see ClangCallConvToLLVMCallConv), we could get away with 6 or 7 bits each, saving this 3-6 bits total, yet have way more than enough room for expansion.

HasRegParm: This field might be possible to eliminate.  According to the GCC RegParm docs (I don't see a clang one?) here (https://gcc.gnu.org/onlinedocs/gcc/x86-Function-Attributes.html), ", the regparm attribute causes the compiler to pass arguments number one to number if they are of integral type in registers EAX, EDX, and ECX instead of on the stack".

It seems that 0 for "RegParm" should be illegal, so I wonder if we can treat "RegParm==0" as "HasRegParm==false" and eliminate storing that.

In my opinion, the 1st one gives us way more room for the future and corrects a possible future bug.  The 2nd is likely a lower touch, though it could possibly change behavior (see discussion here https://www.coreboot.org/pipermail/coreboot/2008-October/040406.html) as regparm(0) is seemingly accepted by both compilers.  I DO note that this comment notes that 'regparm 0' is the default behavior, so I'm not sure what change that would do.

Either way, I suspect this change should be a separate commit (though I would figure making it a pre-req for this patch would be the right way to go).  If you give some guidance as to which you think would be better, I can put a patch together.

-Erich



https://reviews.llvm.org/D22045





More information about the cfe-commits mailing list