vsk added inline comments.
================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359
+ /// condition (i.e. no "&&" or "||").
+ static bool isLeafCondition(const Expr *C);
+
----------------
alanphipps wrote:
> vsk wrote:
> > vsk wrote:
> > > It might be helpful to either require that `C` be the RHS of a logical
> > > binop (e.g. by passing the binop expr in), or to document that
> > > requirement.
> > Given a logical binop like `E = a && !(b || c)`,
> > `isLeafCondition(E->getRHS())` is true. That seems a bit counter-intuitive,
> > because `E->getRHS()` contains another leaf condition. Would it make sense
> > to rename the condition (perhaps to something like
> > 'InstrumentedCondition')? Have I misunderstood what a leaf condition is?
> Background: isLeafCondition() is an auxiliary routine that is used during 1.)
> counter allocation on binop RHS (CodeGenPGO.cpp), 2.) counter instrumentation
> (CodeGenFunction.cpp), and 3.) branch region generation
> (CoverageMappingGen.cpp). In the #3 case, it isn't always looking at the RHS
> of a logical binop but can be used to assess whether any condition is
> instrumented/evaluated.
>
> Given your example condition:
>
> E = a && !(b || c)
>
> You are correct that isLeafCondition(E->getRHS()) will return true, but I
> think it should actually return false here (and bypass logical-NOT), so I
> will adapt this.
>
> However, given a condition that includes an binary operator (like assign):
>
> E = a && (x = !(b || c))
>
> isLeafCondition(E->getRHS()) will also return true, and I think that is the
> correct behavior. Given that, then I agree that maybe isLeafCondition()
> should be renamed to isInstrumentedCondition() since it's not technically
> just leaves that are tracked in the presence of operators.
What is special about the assignment expression nested within "a && (x = !(b ||
c))" that necessitates an extra counter compared to "a && !(b || c)"?
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:46
class SourceMappingRegion {
+ /// Primary Counter that is also used for Branch Regions for "True" branches
Counter Count;
----------------
nit: Please end comments with a '.'
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:523
+
+ RegionStack.emplace_back(Count, FalseCount, StartLoc, EndLoc,
+ FalseCount.hasValue());
----------------
It looks like this sets up a deferred region when a branch count (FalseCount)
is available. Could you explain why, and when the deferred region is expected
to be complete?
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:623
// separate region for each expansion.
+ // Don't do this for branch regions
SourceLocation NestedLoc = getStartOfFileOrMacro(EndLoc);
----------------
Could you explain why there's an exception for branch regions here?
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:724
+ /// Determine whether the given condition folds to true or false
+ bool ConditionFoldsToBool(const Expr *Cond) {
----------------
nit: 'can be constant folded' may be slightly more idiomatic.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:742
+ // function's SourceRegions) because it doesn't apply to any other source
+ // code other than the Condition.
+ if (CodeGenFunction::isLeafCondition(C)) {
----------------
Is there some alternative to pushing and then immediately popping branch
regions? Did you try adding the region directly to the function's
SourceRegions, and find that there was some issue with the more direct approach?
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:749
+ // constant-folding by ensuring that ConstantFoldsToSimpleInteger() in
+ // CodeGenFunction.c always returns false, but that is very heavy-handed.
+ if (ConditionFoldsToBool(C))
----------------
Right, I don't think that alternative is practicable.
================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:659
+void SourceCoverageViewHTML::renderBranchView(raw_ostream &OS,
+ BranchView &BRV,
----------------
Wdyt of displaying branch-taken counts using tooltips? This would match the way
region execution counts are shown, which could be nice, because the information
wouldn't interrupt the flow of source code.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84467/new/
https://reviews.llvm.org/D84467
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
