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

[Alexander Potapenko via Phabricator via cfe-commits]

glider added inline comments.


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


================
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
----------------
jfb wrote:
> 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).
Why? We're passing both PATTERN and PATTERN-OX to FileCheck when testing.
I've checked that replacing `@test_empty_uninit()` on this line with a different function name makes the test fail.


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

https://reviews.llvm.org/D57898