[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 a comment.

Overall this LGTM besides a few nits, and wanting input from @rjmccall.

As follow-ups (which I can take on):

- Handle the case where the types don't match (because `Loc` was adjusted).
- Handle small arrays (and any other cases where the struct stores get broken down into components which still contain a `memcpy`/ `memset`).



================
Comment at: tools/clang/lib/CodeGen/CGDecl.cpp:975
+static bool shouldSplitStructStore(CodeGenModule &CGM, llvm::Constant *Init,
+                                   uint64_t GlobalSize) {
+  // Don't break structures that occupy more than one cacheline.
----------------
You don't use `Init` in the function.


================
Comment at: tools/clang/lib/CodeGen/CGDecl.cpp:979
+  if (CGM.getCodeGenOpts().OptimizationLevel == 0)
+    return false;
+  if (GlobalSize <= SizeLimit)
----------------
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.


================
Comment at: tools/clang/test/CodeGenCXX/auto-var-init.cpp:476
 // CHECK-NEXT:  call void @{{.*}}used{{.*}}%uninit)
 // PATTERN-LABEL: @test_empty_uninit()
+// PATTERN-O0: call void @llvm.memcpy{{.*}} @__const.test_empty_uninit.uninit
----------------
The tests aren't matching labels anymore: `PATTERN-LABEL` is a dead label check now. I think forking things at `-O0` and `-O1` is fine, but I think you want a small `-O0` test (separate from this patch) which does one or two things, and then you want this test to only look at `-O1`. That way you don't need so much stuff changing in the test (and `-O0` is still tested).


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

https://reviews.llvm.org/D57898