[PATCH] D126586: [InstrProf][WIP] Implement boolean counters in coverage

[Paul Kirth via Phabricator via llvm-commits]

paulkirth added a comment.

I think somewhere in this patch there should be a description of the new algorithm. I also don't see any description of the old algorithm, so maybe we should describe both of them so that it's easy to understand the differences in between the different profiling approaches.  For example `llvm/lib/Transforms/Instrumentation/PGOInstrumentation.cpp` describes the MST approach in detail. I don't think it needs to be quite that detailed, but a diagram contrasting where counters are inserted in both approaches would help explain the differences.

Can you also describe how this works w/ the `SwitchStmt`?  It appears to me to just exit early, but the summary seems to indicate that it is part of the current patch. Is that part still WIP? I expected to see similar handling there as we see w/ `IfStmt`.



================
Comment at: clang/lib/CodeGen/CodeGenPGO.cpp:245-246
     // Otherwise, keep track of which branch we're in while traversing.
-    VisitStmt(If);
+    // When boolean counters are enabled and if has an else part,
+    // do not add a counter for IfStmt, but still include in the function hash.
+    if (llvm::EnableBooleanCounters && If->getElse()) {
----------------



================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:1439
+      if (S->getElse())
+        ElseCount = getRegionCounter(S->getElse());
+    } else
----------------
Won't `ElseCount` be uninitialized when `S->getElse() == false`? originally it was always initialized by `subtractCounters`.


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:96-97
   enum CounterKind { Zero, CounterValueReference, Expression };
-  static const unsigned EncodingTagBits = 2;
-  static const unsigned EncodingTagMask = 0x3;
+  static const unsigned EncodingTagBits = 3;
+  static const unsigned EncodingTagMask = 0x7;
   static const unsigned EncodingCounterTagAndExpansionRegionTagBits =
----------------
Do these changes need to be explained in the comment above?


================
Comment at: llvm/include/llvm/ProfileData/Coverage/CoverageMapping.h:152
   ExprKind Kind;
+
   Counter LHS, RHS;
----------------
nit: added whitespace


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

https://reviews.llvm.org/D126586