[PATCH] D46357: [GCOV] Emit the writeout function as nested loops of global data.

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

In https://reviews.llvm.org/D46357#1085440, @wmi wrote:

> The generated code looks great. We may want to do the similar thing in @__llvm_gcov_flush. It generated a bunch of memset in straight line code in a loop of CountersBySP.
>  call void @llvm.memset.p0i8.i64(i8* align 16 bitcast ([2 x i64]* @__llvm_gcov_ctr to i8*), i8 0, i64 16, i1 false)
>  call void @llvm.memset.p0i8.i64(i8* align 16 bitcast ([12 x i64]* @__llvm_gcov_ctr.6 to i8*), i8 0, i64 96, i1 false)
>  call void @llvm.memset.p0i8.i64(i8* align 16 bitcast ([2 x i64]* @__llvm_gcov_ctr.7 to i8*), i8 0, i64 16, i1 false)
>  ...


Sure, but let's do that in a follow-up? It isn't hitting scaling limits nearly as rapidly from my testing.

In https://reviews.llvm.org/D46357#1085373, @davidxl wrote:

> Can you add a new test case testing the new emitted code patterns?


Yep (see my response to dblaikie's comment below too).



================
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:913
+  SmallVector<Constant *, 8> FileInfos;
+  for (int i : llvm::seq<int>(0, CUNodes->getNumOperands())) {
+    auto *CU = cast<DICompileUnit>(CUNodes->getOperand(i));
----------------
dblaikie wrote:
> Maybe worth using 'zip' here to iterate the CUNodes (it does support iterators rather than using getOperand(i))? Might be easier, I guess, if llvm::seq supported more defaults (like defaulting to int, starting at zero, and unbounded (no need to specify an end value if the thing you zip it with is constrained anyway))
I'd like to do this in a follow-up change if I can? I already did more surgery here than I probably should have. Happy to iterate some on cleaner loops though.


================
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:973
+        ConstantArray::get(EmitArcsCallArgsArrayTy, EmitArcsCallArgsArray),
+        "__llvm_internal_gcov_emit_arcs_args");
+    EmitArcsCallArgsArrayGV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
----------------
dblaikie wrote:
> Is this tested? (the test case only seems to have function_args)
> 
> (also I'm not sure I really understand this bit of the code - it's creating a global variable with a fixed name, but it's doing so inside the loop over all CUs? (would this create duplicate GVs? Do they somehow get renamed to be unique?) Is this code assuming there will only be one non-module-skeleton CU?)
I think we always have both? The test only looks at one. I honestly don't understand the *contents* of this well enough to write a test. I now test the writeout which at least clearly shows that the structure ends up being sound here.

Regarding the global variable -- I was assuming they get renamed like SSA values. Their names aren't significant as they don't have linkage. But I can just make the names unique constructively, as that seems cleaner anyways.


================
Comment at: llvm/test/Transforms/GCOVProfiling/function-numbering.ll:22-24
+; GCDA: @__llvm_internal_gcov_emit_function_args = internal unnamed_addr constant
+; GCDA-SAME: { i32 0, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @[[FOO]]
+; GCDA-SAME: { i32 1, i8* getelementptr inbounds ([4 x i8], [4 x i8]* @[[BAZ]]
----------------
dblaikie wrote:
> I'd expect this test should probably test the contents of the gcov_writeout function? Nothing else appears to be testing that code? (given that you changed it significantly & no other tests need updating)
Yeah, I was trying to avoid adding large amounts of new testing but the testing seems super bad here...

I've added a full test of the writeout function now.


Repository:
  rL LLVM

https://reviews.llvm.org/D46357