[PATCH] D51974: [GCOV] Handle correctly multiple CUs when profiling

[Marco Castelluccio via Phabricator via llvm-commits]

marco-c accepted this revision.
marco-c added a comment.
This revision is now accepted and ready to land.

Very nice simplification, I hope this won't introduce regressions.
I think we should test the patch on Mozilla CI before landing, so we can be reasonably confident that it doesn't break things.



================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:135
+      CUToFuncs;
+  CUFunctionsTy CUToFunctions;
 };
----------------
It's a little confusing to have two variable names which are so similar. Maybe call the first CUToGCOVFunctions and the second CUToFunctions?


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:479
   this->TLI = &TLI;
+  CUToFunctions = getCUToFunctions(M);
   Ctx = &M.getContext();
----------------
Nit: Maybe keep the same convention of using `this->` for member variables?


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:755
+  FTy = FunctionType::get(Type::getVoidTy(*Ctx), false);
+  Type *Params[] = {PointerType::get(FTy, 0), PointerType::get(FTy, 0)};
+  FTy = FunctionType::get(Builder.getVoidTy(), Params, false);
----------------
There are some formatting changes here and there, did you make them with clang format?
Next time, can you do the formatting changes after review, so I don't have to see them


================
Comment at: test/Transforms/GCOVProfiling/function-numbering.ll:27
+; GCDA:         call void @llvm_gcda_emit_function({{.*}} @[[BAZ]]{{.*}})
+; GCDA:    ret void
 
----------------
Do we have another test for the case where CUToCounters is not empty? I assume we don't.
It would be nice to have it, even a basic one, so I can also see how the generated code looks like now.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D51974