[clang] [clang][NFC] Regroup declarations in `Sema` (PR #82217)

Erich Keane via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 20 08:47:25 PST 2024


erichkeane wrote:

> @erichkeane
> 
> > That said, waiting until after 18 is perhaps a good diea.
> 
> Resolving merge conflicts that will arise in the meantime is not going to be trivial, but should be doable in a reasonable time. So I'm willing to wait.
> 

I'm glad to hear that!  I'm hopeful it isn't TOO much of a delay and not too much work. Sema.h changes are at least pretty innocuous/small.

> > I MIGHT suggest private followed by public? It is a not-uncommon pattern I've seen to have a private 'helper' class(or data member) defined inline, that is then used by inline-defined public functions.
> 
> I don't mind either way, as long as we don't inserting a couple of private members in the middle of public section.
> 

I agree with the goal as well!

> > I wouldn't mind some sort of 'static_assert' to ensure that this doesn't accidentally increase the size of Sema
> 
> Checking on Linux, `sizeof(Sema)` is 18824 bytes at moment. This patch increases that by 24 bytes, to `18848`. I don't deem it significant enough to care for an object this big, but I can claw it back if this is considered important.
>

Urgh, and double-urgh. Yeah, not big enough/important enough, we only ever instantiate a handful of Sema objects anyway at most.
 
> > or cause some sort of pessimization for layout. I realize we're not particularly concerned about the size, but I could imagine goofy things going on.
> 
> If there was an intent to put some data members first to improve cache hits, I think it has been lost by this point. Our first non-static data members are `OpenCLOptions OpenCLFeatures` and `FPOptions CurFPFeatures` at the moment. Since this patch puts generic Sema stuff (`Sema.cpp`) first, opportunity to put widely used data members first is still there. As I mentioned in the description, follow-up patches that improve ordering in each particular section are expected.

"Commonly used" stuff together perhaps has some value for cache-locality reasons (as well as any other "closely related" stuff with itself), but I don't have a good way to measure that so 'best effort' here is probably good enough/the changes you're intending a good enough attempt.



https://github.com/llvm/llvm-project/pull/82217


More information about the cfe-commits mailing list