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

[Alan Phipps via Phabricator via llvm-commits]

alanphipps added inline comments.


================
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). */
----------------
ellis wrote:
> Do we also need to bump the version for the raw files since we added to its header?
Thank you, I totally overlooked this.


================
Comment at: llvm/include/llvm/IR/IntrinsicInst.h:1335
+  // The number of bytes used for MCDC bitmaps for the instrumented function.
+  ConstantInt *getNumBitmapBytes() const;
 };
----------------
ellis wrote:
> 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?
I refactored this into a base class for MCDC and one for Counters.


================
Comment at: llvm/lib/Transforms/Instrumentation/InstrProfiling.cpp:795
 
+void InstrProfiling::lowerMCDCTestVectorBitmapUpdate(
+    InstrProfMCDCTVBitmapUpdate *Update) {
----------------
alanphipps wrote:
> ellis wrote:
> > 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).
> Hmm. Right, I do test the lowering using tests I added in `clang/test/Profile` (It's in the clang-specific review, but obviously it has overlap with what is done in this portion), but I overlooked adding anything specific to `llvm/test/Instrumentation/InstrProfiling`. 
> 
> I'll look into adding some additional tests for this, as well as for the intrinsics.
I added to `llvm/test/Instrumentation/InstrProfiling` and also an intrinsics case to `clang/test/Profile` (in the clang-specific MC/DC review).



================
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");
+
----------------
ellis wrote:
> 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.
Thanks, noted. -- I did add the name here because it was definitely useful in debugging.  I'm going to keep the name for now unless you or others feel strongly.


================
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);
----------------
ellis wrote:
> 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.
I looked into this, and I //think //it is possible to enable a atomic RMW version using Or, similar to what is done for Add.  It isn't something that I'm particularly interested in at this time, but I can definitely do it as a future patch.


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

https://reviews.llvm.org/D138846