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

[Jessica Paquette via Phabricator via llvm-commits]

paquette added a comment.

I added a few inline comments, but it looks like this patch has been in review for a while.

@ellis do you have any issues with this patch going forward?



================
Comment at: compiler-rt/lib/profile/InstrProfilingFile.c:259
+
+  /* BIAS MODE not supported yet for Bitmap (MCDC). */
+
----------------
Would it be appropriate to issue a warning to a user trying to use both MCDC + bias mode?


================
Comment at: compiler-rt/lib/profile/InstrProfilingMerge.c:187
+        return 1;
+      for (unsigned I = 0; I < NB; I++)
+        DstBitmap[I] |= SrcBitmap[I];
----------------
Could you explain what exactly you are doing as you iterate over the number of bitmap bytes?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:1494
+
+  // The number of bytes used for MCDC bitmaps for the instrumented function.
+  ConstantInt *getNumBitmapBytes() const {
----------------
Doxygen?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:1521
+
+  // The index of the bitmap that this instruction acts on.
+  ConstantInt *getBitmapIndex() const {
----------------
Doxygen?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:1526
+
+  Value *getMCDCCondBitmapAddr() const {
+    return cast<Value>(const_cast<Value *>(getArgOperand(4)));
----------------
This could use a Doxygen comment too?


================
Comment at: llvm/include/llvm/Transforms/Instrumentation/InstrProfiling.h:142
+
+  /// Create the region bitmaps.
+  GlobalVariable *createRegionBitmaps(InstrProfMCDCBitmapInstBase *Inc,
----------------
Can you elaborate on this comment a bit more?

1) Is this returning a GlobalVariable representing the region bitmaps?
2) What are the parameters?


================
Comment at: llvm/lib/ProfileData/InstrProfReader.cpp:452
+        if ((Line++)->getAsInteger(0, BitmapByte))
+          return error(instrprof_error::malformed, "bitmap byte is invalid");
+        Record.BitmapBytes.push_back(BitmapByte);
----------------
you're specifically trying to get an integer here, right?


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:743
+
+  // TODO: May need to support runtime counter relocation for bitmaps prior to
+  // upstreaming. See getCounterAddress().
----------------
Is this TODO still relevant?


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:882
+  // Store the updated temporary value back to the stack.
+  //  store i32 %3, ptr %mcdc.addr, align 4
+  Builder.CreateStore(Result, MCDCCondBitmapAddr);
----------------
I like these comments. :)


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1047
+  bool UseComdat = (NeedComdat || TT.isOSBinFormatELF());
+  if (UseComdat) {
+    StringRef GroupName = TT.isOSBinFormatCOFF() && DataReferencedByCode
----------------
How about reduce indentation here by using an early return?

```
if (!UseComdat)
  return;
StringRef GroupName = ...;
```


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1077
     Linkage = GlobalValue::InternalLinkage;
-
+  }
   // Due to the limitation of binder as of 2021/09/28, the duplicate weak
----------------
Don't need to add braces here


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1150
+    return PD.RegionBitmaps;
 
+  auto *BitmapPtr = setupProfileSection(Inc, IPSK_bitmap);
----------------
I'm a little surprised this doesn't call `createRegionBitmaps`. Why is that? Should this have a different name?


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1182
+GlobalVariable *
+InstrProfiling::getOrCreateRegionCounters(InstrProfCntrInstBase *Inc) {
+  GlobalVariable *NamePtr = Inc->getName();
----------------
Similar to above, I'm surprised there's no code sharing with createRegionCounters here.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:1238
+                                        InstrProfMCDCBitmapParameters *Params) {
+  if (DebugInfoCorrelate)
+    return;
----------------
Comment?


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

https://reviews.llvm.org/D138846