alanphipps marked an inline comment as done. alanphipps added inline comments.
================ Comment at: clang/lib/CodeGen/CodeGenFunction.h:4359 + /// condition (i.e. no "&&" or "||"). + static bool isLeafCondition(const Expr *C); + ---------------- vsk wrote: > 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. I just realized I didn't make this name change to isInstrumentedCondition(). I will do that. 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
