Compare Expressions: Fix the conditions for coalescing a node with its parent #943
Conversation
We had erroneously added the condition that to coalesce nodes the binary operators of the parent and child node must be equal. This fails for the case where we first constant-fold nodes having a differen operator than the parent and then need to coalesce the constant folded node into the parent. Fixes issue #932
Done. |
We can coalesce a BinaryNode if any one of the following conditions are true: 1. The parent has the same operator as the current node OR 2. The current node has just one child (for example, as a result of constant folding) and the parent and current operators are commutative and associative.
mgrang
changed the title
|
Some notes from the PR review discussion:
|
clang/lib/AST/PreorderAST.cpp
Outdated
| // 1. The parent has the same operator as the current node OR | ||
| // 2. The current node has just one child (for example, as a result of | ||
| // constant folding) and the parent and current operators are commutative and | ||
| // associative. |
There was a problem hiding this comment.
My thoughts are as follows:
a) Should the condition 1 above not be "parent has the same operator as the current node, and the operator is commutative and associative" ?
b) If we implement constant folding to be complete in itself, then the current node can never have just one child because of constant folding. It is possible that the current node will have just one child if the operator at the current node is a unary operator.
There was a problem hiding this comment.
My thoughts are as follows:
a) Should the condition 1 above not be "parent has the same operator as the current node, and the operator is commutative and associative" ?
Could you elaborate why this should not be the case? We should not try to coalesce different operators and coalescing only makes sense if the operator is commutative and associative because after coalescing we also need to sort the nodes.
---> I meant the same thing. It seems like my question was confusing. The main point of my question is that the comment on lines 39 thro' 42 in the file PreorderAST.cpp is a bit unclear about what you just explained in the sentence above.
b) If we implement constant folding to be complete in itself, then the current node can never have just one child because of constant folding. It is possible that the current node will have just one child if the operator at the current node is a unary operator.
We have now implemented constant folding to be complete in itself. As part of constant folding we now invoke this method to coalesce the node. So we need this condition here. Otherwise we would end up duplicating logic to coalesce nodes.
---> Sounds good.
We can coalesce a BinaryNode if any one of the following conditions are
true:
constant folding) and the parent and current operators are commutative and
associative.
Fixes issue #932