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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits