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

[Erich Keane via cfe-commits]

erichkeane added inline comments.

================
Comment at: include/clang/CodeGen/CGFunctionInfo.h:479
@@ +478,3 @@
+  /// Whether this function saved caller registers.
+  unsigned NoCallerSavedRegs : 1;
+
----------------
majnemer wrote:
> aaron.ballman wrote:
> > erichkeane wrote:
> > > aaron.ballman wrote:
> > > > erichkeane wrote:
> > > > > 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
> > > > > 
> > > > I think that `unsigned ASTCallingConvention : 8;` can be safely reduced. This tracks a `clang::CallingConv` value, the maximum of which is 15. So I think the way to do this is to reduce ASTCallingConvention from 8 to 7 bits and then pack yours in as well.
> > > Ah! I missed that this was the case.  That said, it could likely be reduced to 6 if we really wished (currently 16 items, 6 gives us room for 32).  Perhaps something to keep in our pocket for the next time someone needs a bit or two here.
> > > 
> > > 
> > > I'll update the diff for Amjad if possible.
> > I'm on the fence about 6 vs 7 and see no harm in reducing it to either value, but have a *very* slight preference for 7 so that the bit-field grouping continues to add up to 32-bits. However, it's your call.
> I'd go with 6 and have another bitfield, `unsigned UnusedBits : 1;`
> 
> We use this paradigm elsewhere.
That seems like a solid solution.  I'll add it to another diff and give it to Amjad to update this patch.  Not sure if he has the ability to (or if it is too late), but I don't have permissions to alter this commit for him.


https://reviews.llvm.org/D22045