[PATCH] D57898: [RFC] Split constant structures generated by -ftrivial-auto-var-init when emitting initializators

Fri Feb 8 02:29:52 PST 2019

glider marked 2 inline comments as done.
glider added inline comments.

================
Comment at: tools/clang/lib/CodeGen/CGDecl.cpp:1143
+    const llvm::StructLayout *Layout =
+        CGM.getDataLayout().getStructLayout(cast<llvm::StructType>(Ty));
+    for (unsigned i = 0; i != constant->getNumOperands(); i++) {
----------------
jfb wrote:
> I think you need a heuristic here, where you don't emit multiple stores if `getNumOperands` is greater than some number. In that case you'd keep doing memcpy instead. Note that a struct containing sub-structs (or arrays) should also count the number of sub-structs (so just checking `getNumOperands` isn't sufficient).
> 
> Other parts of this code chose 6 stores...
Do we ever need to not split these stores? Can't the  MemCpyOpt pass take care of them later on?

================
Comment at: tools/clang/lib/CodeGen/CGDecl.cpp:1765
+  emitStoresForConstant(CGM, D, Loc, isVolatile, Builder, constant,
+                        /*forInit*/ false);
 }
----------------
jfb wrote:
> Can you explain why this case needs to be different? IIUC it's because the types can differ which makes the struct's elements not match up the ones from the constant in the loop above? I think the code above should automatically handle that case instead of passing in a flag here.
> 
> You also definitely need to test the case where that happens :)
I just thought we shouldn't break the constants that we didn't create with -ftrivial-var-auto-init.
I'll take a closer look though (indeed, this crashes on one of the structs from the Linux kernel)

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

https://reviews.llvm.org/D57898