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

[Ellis Hoag via Phabricator via llvm-commits]

ellis added a comment.

Please correct me if I'm wrong, but my understanding is that we have a single bitvector per expression (or function?) to track coverage. And that these bitvectors are at most 64 bits long. Is the number of these bitvectors per function known at compile time? I think yes because you are allocating the `IPSK_bitmap` section at compile time.

If this is the case then I would suggest extending the `IPSK_cnts` rather than creating a new `IPSK_bitmap` section. There is lots of logic to track the location of the `IPSK_cnts` and correlate its values to the correct functions (this is the main feature of Debug Info Correlation). Instead, we could allocate more data for function counters and say the last `N` bytes are for the MC/DC bitvectors. The lowered code would know where to write the data because they are given the address and the raw profile readers would know when to expect this data at the end of the counters because they know `N`.
This would simplify `INSTR_PROF_DATA` because we would only need `NumBitmapBytes` and not `RelativeBitmapPtr`, we wouldn't need to extend the raw header, we wouldn't need the `IPSK_bitmap` section, and the `llvm.instrprof.mcdc.` intrinsics could be more closely aligned to the other `llvm.instrprof.` intrinsics.

I'm open to other ideas/arguments so let me know what you think.



================
Comment at: compiler-rt/include/profile/InstrProfData.inc:661
 /* Raw profile format version (start from 1). */
 #define INSTR_PROF_RAW_VERSION 8
 /* Indexed profile format version (start from 1). */
----------------
Do we also need to bump the version for the raw files since we added to its header?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:1335
+  // The number of bytes used for MCDC bitmaps for the instrumented function.
+  ConstantInt *getNumBitmapBytes() const;
 };
----------------
I don't think this field applies to other instrprof intrinsics. Can we create a new base class exclusively for MCDC intrinsics and add this method there instead?


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:1397
+/// This represents the llvm.instrprof.mcdc.parameters intrinsic.
+class InstrProfMCDCParameters : public InstrProfInstBase {
+public:
----------------
You'll also want to add these new intrinsics to `llvm/docs/LangRef.rst` .


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:6979
     llvm_unreachable("instrprof failed to lower a value profiling call");
+  case Intrinsic::instrprof_mcdc_tvbitmap_update:
+    llvm_unreachable("instrprof failed to lower an mcdc tvbitmap update");
----------------
Did you mean to add `::instrprof_mcdc_parameters` here too?


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:795
 
+void InstrProfiling::lowerMCDCTestVectorBitmapUpdate(
+    InstrProfMCDCTVBitmapUpdate *Update) {
----------------
I'm not seeing any tests in `llvm/test/Instrumentation/InstrProfiling/` to test that this code was lowered correctly. Can you add some?

It would also be good to add tests in `clang/test/Profile` or `llvm/test/Transforms/PGOProfile/` to make sure these intrinsics are added to the correct locations (I actually don't see where `InstrProfMCDCTVBitmapUpdate` is created).


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:807
+  //  %mcdc.temp = load i32, ptr %mcdc.addr, align 4
+  auto *Temp = Builder.CreateLoad(Int32Ty, MCDCCondBitmapAddr, "mcdc.temp");
+
----------------
I've been told that register names could increase memory usage and it's best not to add them unless they are very useful for debugging.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:833-843
+  // Load profile bitmap byte.
+  //  %mcdc.bits = load i8, ptr %4, align 1
+  auto *Bitmap = Builder.CreateLoad(Int8Ty, BitmapByteAddr, "mcdc.bits");
+
+  // Perform logical OR of profile bitmap byte and shifted bit offset.
+  //  %8 = or i8 %mcdc.bits, %7
+  auto *Result = Builder.CreateOr(Bitmap, ShiftedVal);
----------------
There is an option to make the increment code atomic. See `lowerIncrement()`. Are you interested in supporting this option? Though I'm not sure if there is an atomic operation that could do this.


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

https://reviews.llvm.org/D138846