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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
