[PATCH] D57898: CodeGen: Fix PR40605: split constant structures generated by -ftrivial-auto-var-init when emitting initializators

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.


================
Comment at: tools/clang/lib/CodeGen/CGDecl.cpp:979
+  if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+    return false;
+  if (GlobalSize <= SizeLimit)
----------------
rjmccall wrote:
> glider wrote:
> > jfb wrote:
> > > The general 64-byte heuristic is fine with me. It's just a bit weird to special-case `-O0`, but we do it elsewhere too (and it keeps the current status-quo of letting the backend decide what to do). From that perspective I'm fine with it.
> > > 
> > > @rjmccall do you have a strong preference either way?
> > > 
> > > One small nit: can you use `ByteSize` instead of just `Size`? Makes it easier to figure out what's going on in code IMO.
> > I don't have a strong opinion on variable naming, but this:
> > ```
> >  974 static bool shouldSplitStructStore(CodeGenModule &CGM,
> >  975                                    uint64_t GlobalByteSize) {
> >  976   // Don't break structures that occupy more than one cacheline.
> >  977   uint64_t ByteSizeLimit = 64;
> >  978   if (CGM.getCodeGenOpts().OptimizationLevel == 0)
> >  979     return false;
> >  980   if (GlobalByteSize <= ByteSizeLimit)
> >  981     return true;
> >  982   return false;
> > ```
> > looks a bit more heavyweight than the previous function, in which we also have `GlobalSize` and `SizeLimit`.
> What are the cases we actually want to make sure we emit as separate stores?  Basically just really small types that happen to be wrapped up in several layers of struct for abstraction purposes, right?  Maybe this heuristic should primarily consider element counts (and types) and use overall sizes at the top level.
Yes, small types. 


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57898/new/

https://reviews.llvm.org/D57898