[PATCH] D138846: MC/DC in LLVM Source-Based Code Coverage: LLVM back-end and compiler-rt

[Alan Phipps via Phabricator via llvm-commits]

alanphipps marked 16 inline comments as done.
alanphipps added inline comments.


================
Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:259
+
+  /* BIAS MODE not supported yet for Bitmap (MCDC). */
+
----------------
paquette wrote:
> Would it be appropriate to issue a warning to a user trying to use both MCDC + bias mode?
Yes, this is related to runtime counter relocation. I will issue a warning from the InstrProfiling pass if this is seen (along with a test case).


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1150
+    return PD.RegionBitmaps;
 
+  auto *BitmapPtr = setupProfileSection(Inc, IPSK_bitmap);
----------------
paquette wrote:
> I'm a little surprised this doesn't call `createRegionBitmaps`. Why is that? Should this have a different name?
createRegionBitmaps() is called via setupProfileSection() just below. I'll add a brief comment here to (hopefully) make it clearer.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1182
+GlobalVariable *
+InstrProfiling::getOrCreateRegionCounters(InstrProfCntrInstBase *Inc) {
+  GlobalVariable *NamePtr = Inc->getName();
----------------
paquette wrote:
> Similar to above, I'm surprised there's no code sharing with createRegionCounters here.
The goal was to abstract what I could through setupProfileSection(), since sections have to be created for both counters and bitmaps, while still keeping the Counter and Bitmap concepts distinct.  Frankly, I think this whole pass could eventually benefit from a rewrite owing to its evolution, but I attempted not to make it worse.


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

https://reviews.llvm.org/D138846