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:
> > 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)"?
> I'm exempting the logical NOT operator basically to match the functionality
> of other coverage vendors (Bullseye, GCOV). It's a simplistic operator in
> the sense that (as far as I can tell) it only affects the sense of the
> generated branch on a condition.
>
> As for the assignment, we're effectively creating a new condition that is
> evaluatable (even if a branch may technically not be generated), and so
> creating a counter for it is more interesting (and matches other vendor
> behavior).
>
> But I'm open to persuasion. We could just be more conservative and create
> counters for logical NOT, but I think there's value in matching some of the
> expected behavior of other vendors. This is also something we could continue
> to fine-tune in future patches.
I agree that there isn't a need to insert a fresh counter to accommodate a
logical NOT operator in a condition. But I don't see how an assignment
expression effectively creates a new, evaluatable condition. It seems like the
count for the assignment expr can be derived by looking up the count for its
parent region.
At this stage I'm more interested in understanding the high-level design. If
the question of whether/not to add fresh counters for assignment exprs in a
condition is effectively about optimization, then I'm fine with tabling it.
================
Comment at: clang/lib/CodeGen/CoverageMappingGen.cpp:523
+
+ RegionStack.emplace_back(Count, FalseCount, StartLoc, EndLoc,
+ FalseCount.hasValue());
----------------
alanphipps wrote:
> vsk wrote:
> > 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?
> Does adding a region to the RegionStack always imply setting up a deferred
> region that needs to be completed? My intention was to add the branch region
> to the RegionStack but not to setup or complete a deferred region for branch
> regions.
>
> Branch regions are straightforward since we already know start location and
> end location when they are created, but I have probably misunderstood what a
> deferred region implies.
>
>
Oops, I see now that this selects a different SourceMappingRegion constructor
than the one I'd thought of. In general, no, adding a region to RegionStack
doesn't always entail setting up a deferred region. It looks like you're not
doing that here. (An aside: deferred regions were introduced to fix line
execution counts for lines following terminator statements, like a 'return'.)
================
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)) {
----------------
alanphipps wrote:
> vsk wrote:
> > 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?
> The push/pop combo does look strange, and you're right -- I can just add it
> directly to SourceRegions, but the algorithm in popRegions() will do
> additional things like adjust the start/end location if the region spans a
> nested file depth. I think that's still useful to do. Another option is I
> could factor that algorithm out of popRegions() into a new function and also
> call that from createBranchRegions().
If it's possible/straightforward to factor the region start/end adjustment code
out of popRegions(), that would be really nice. If not, the current approach
looks reasonable.
================
Comment at: llvm/tools/llvm-cov/SourceCoverageViewHTML.cpp:659
+void SourceCoverageViewHTML::renderBranchView(raw_ostream &OS,
+ BranchView &BRV,
----------------
alanphipps wrote:
> vsk wrote:
> > 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.
> I think using tooltips is a great idea, and I did look into it. Ultimately,
> I decided to defer that work to a future patch to allow for some time to
> research a good way to properly distinguish the branch-taken counts from the
> region counts.
Sounds reasonable. Can the branch coverage text/html output should be opt-in
via a cl::opt until it's finalized? That should make it possible to iterate on
the format without changing what users see.
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
