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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

Some minor/idle comments



================
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));
----------------
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))


================
Comment at: llvm/lib/Transforms/Instrumentation/GCOVProfiling.cpp:973
+        ConstantArray::get(EmitArcsCallArgsArrayTy, EmitArcsCallArgsArray),
+        "__llvm_internal_gcov_emit_arcs_args");
+    EmitArcsCallArgsArrayGV->setUnnamedAddr(GlobalValue::UnnamedAddr::Global);
----------------
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?)


================
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]]
----------------
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)


Repository:
  rL LLVM

https://reviews.llvm.org/D46357