xazax.hun accepted this revision.
xazax.hun added inline comments.
================
Comment at: clang/lib/Analysis/FlowSensitive/Transfer.cpp:516
+ // assigned to it.
+ Visit(&SubExpr);
+ if (auto *Val = dyn_cast_or_null<BoolValue>(
----------------
sgatev wrote:
> xazax.hun wrote:
> > sgatev wrote:
> > > xazax.hun wrote:
> > > > Could you elaborate on when would this happen? I'd expect the traversal
> > > > to always visit the predecessor basic blocks first and within a basic
> > > > block always visit subexpressions first. So I'd be quite surprised if
> > > > there is a subexpression we did not visit.
> > > From what I've seen, logic operators influence the structure of the CFG
> > > through additional basic blocks and terminators, but their sub-expression
> > > operators are not added directly in the basic blocks.
> > >
> > > For example:
> > > ```
> > > void test(bool a, bool b, bool c) {
> > > bool d = a && (b || c);
> > > }
> > > ```
> > >
> > > results in:
> > > ```
> > >
> > > void test(bool a, bool b, bool c)
> > > [B5 (ENTRY)]
> > > Succs (1): B4
> > >
> > > [B1]
> > > 1: [B4.2] && ([B3.2] || [B2.2])
> > > 2: bool d = a && (b || c);
> > > Preds (3): B2 B3 B4
> > > Succs (1): B0
> > >
> > > [B2]
> > > 1: c
> > > 2: [B2.1] (ImplicitCastExpr, LValueToRValue, _Bool)
> > > Preds (1): B3
> > > Succs (1): B1
> > >
> > > [B3]
> > > 1: b
> > > 2: [B3.1] (ImplicitCastExpr, LValueToRValue, _Bool)
> > > T: [B3.2] || ...
> > > Preds (1): B4
> > > Succs (2): B1 B2
> > >
> > > [B4]
> > > 1: a
> > > 2: [B4.1] (ImplicitCastExpr, LValueToRValue, _Bool)
> > > T: [B4.2] && ...
> > > Preds (1): B5
> > > Succs (2): B3 B1
> > >
> > > [B0 (EXIT)]
> > > Preds (1): B1
> > > ```
> > >
> > > So, when we evaluate `a && (b || c)` in `B1`, the sub-expression `b || c`
> > > has not been evaluated yet. I updated the comment in the code to make
> > > that more clear.
> > I understand the structure of the CFG and also understand that certain
> > subexpressions are in different basic blocks. But I still don't understand
> > why would `b || c` be not evaluated when we evaluate `a && (b || c)`.
> >
> > The operator `&&` in your example is evaluated in `B1`. The operator `||`
> > is evaluated in `B3`. `B3` is a predecessor of `B1`, so if we process the
> > CFG in reverse-post order, we should visit `B3` before `B1`.
> >
> > I am pretty sure if the traversal is well-written we should have the `||`
> > evaluated before we visit `B1`.
> >
> > I suspect that something different might be going on. Is it possible that
> > you want to evaluate the `&&` in `B4`?
> >
> > Note that `&&` is a terminator there because of the short-circuiting. So at
> > that point we should NOT ask for the value of `||`.
> > The operator `||` is evaluated in `B3`.
>
> I don't think that's the case. Similar to your observation about `&&` being
> a terminator in `B4`, I believe that `||` is a terminator in `B3`.
>
> I interpret `B3` as the "`a` is true" branch. It still doesn't contain
> information about `b` and `c` which might be subject to short-circuiting.
>
> Does that make sense?
Oh, I see now! Sorry, I somehow assumed `Visit` is recursive. But you only
visit the top level operator not the whole subexpression. Yeah, I understand
now, thanks! This looks good to me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D121455/new/
https://reviews.llvm.org/D121455
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
