From 6087a72b5cf1e2e62c8a9ba46256a2f9e3382ff7 Mon Sep 17 00:00:00 2001 From: Mandeep Singh Grang Date: Mon, 16 Nov 2020 16:05:41 -0800 Subject: [PATCH 1/6] Compare Expressions: Remove erroneous condition on coalescing nodes 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 --- clang/lib/AST/PreorderAST.cpp | 6 +----- .../widened-bounds-semantic-compare.c | 12 ++++++++++++ 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/clang/lib/AST/PreorderAST.cpp b/clang/lib/AST/PreorderAST.cpp index 83c6fecbd1df..ea707d3ab5de 100644 --- a/clang/lib/AST/PreorderAST.cpp +++ b/clang/lib/AST/PreorderAST.cpp @@ -42,14 +42,10 @@ void PreorderAST::RemoveNode(Node *N, Node *Parent) { // We will remove a BinaryNode only if its operator is equal to its // parent's operator and the operator is commutative and associative. if (auto *B = dyn_cast(N)) { - assert(B->Opc == P->Opc && - "BinaryNode operator must equal parent operator"); - assert(B->IsOpCommutativeAndAssociative() && "BinaryNode operator must be commutative and associative"); - if (B->Opc != P->Opc || - !B->IsOpCommutativeAndAssociative()) { + if (!B->IsOpCommutativeAndAssociative()) { SetError(); return; } diff --git a/clang/test/CheckedC/inferred-bounds/widened-bounds-semantic-compare.c b/clang/test/CheckedC/inferred-bounds/widened-bounds-semantic-compare.c index 39c4ed645326..0a6ed4468e2c 100644 --- a/clang/test/CheckedC/inferred-bounds/widened-bounds-semantic-compare.c +++ b/clang/test/CheckedC/inferred-bounds/widened-bounds-semantic-compare.c @@ -169,3 +169,15 @@ void f14(int i) { // CHECK: [B1] // CHECK-NOT: upper_bound(p) } + +void f15(_Nt_array_ptr p : count(6)) { + if (*(p + (1 * (2 * 3)))) + {} + +// CHECK: In function: f15 +// [B2] +// 1: _Nt_array_ptr p : count(6) = "a"; +// 2: *(p + (1 * (2 * 3))) +// [B1] +// upper_bound(p) = 1 +} From 35006fb7e8bfd97976f92a00110f1578d62c1952 Mon Sep 17 00:00:00 2001 From: Mandeep Singh Grang Date: Mon, 16 Nov 2020 16:09:07 -0800 Subject: [PATCH 2/6] Fixed comment --- clang/lib/AST/PreorderAST.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clang/lib/AST/PreorderAST.cpp b/clang/lib/AST/PreorderAST.cpp index ea707d3ab5de..1bf6e16bcf55 100644 --- a/clang/lib/AST/PreorderAST.cpp +++ b/clang/lib/AST/PreorderAST.cpp @@ -39,8 +39,8 @@ void PreorderAST::RemoveNode(Node *N, Node *Parent) { return; } - // We will remove a BinaryNode only if its operator is equal to its - // parent's operator and the operator is commutative and associative. + // We will remove a BinaryNode only if its operator is commutative and + // associative. if (auto *B = dyn_cast(N)) { assert(B->IsOpCommutativeAndAssociative() && "BinaryNode operator must be commutative and associative"); From cebcd9676e2e9314628baccdb809d03b4498d4fa Mon Sep 17 00:00:00 2001 From: Mandeep Singh Grang Date: Tue, 17 Nov 2020 11:53:04 -0800 Subject: [PATCH 3/6] Fix the conditions for coalescing a node with its parent 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. --- clang/include/clang/AST/PreorderAST.h | 15 +++++-- clang/lib/AST/PreorderAST.cpp | 59 +++++++++++++-------------- 2 files changed, 40 insertions(+), 34 deletions(-) diff --git a/clang/include/clang/AST/PreorderAST.h b/clang/include/clang/AST/PreorderAST.h index c8c90a6d4027..63885a49e01c 100644 --- a/clang/include/clang/AST/PreorderAST.h +++ b/clang/include/clang/AST/PreorderAST.h @@ -88,11 +88,20 @@ namespace clang { // @param[in] Parent is the parent of the node to be added. void AddNode(Node *N, Node *Parent); - // Move the children (if any) of node N to its parent and then remove N. - // @param[in] N is the current node. + // Coalesce the BinaryNode B with its parent P. This involves moving the + // children (if any) of node B to its parent and then removing B. + // @param[in] B is the current node. B should be a BinaryNode. // @param[in] P is the parent of the node to be removed. P should be a // BinaryNode. - void RemoveNode(Node *N, Node *P); + void CoalesceNode(BinaryNode *B, BinaryNode *P); + + // Determines if a BinaryNode could be coalesced into its parent. + // @param[in] B is the current node. B should be a BinaryNode. + // @param[in] P is the parent of the node to be removed. P should be a + // BinaryNode. + // @return Return true if B can be coalesced into its parent P, false + // otherwise. + bool CanCoalesceNode(BinaryNode *B, BinaryNode *P); // Recursively coalesce binary nodes having the same commutative and // associative operator. diff --git a/clang/lib/AST/PreorderAST.cpp b/clang/lib/AST/PreorderAST.cpp index 1bf6e16bcf55..8ade8922966c 100644 --- a/clang/lib/AST/PreorderAST.cpp +++ b/clang/lib/AST/PreorderAST.cpp @@ -29,47 +29,48 @@ void PreorderAST::AddNode(Node *N, Node *Parent) { } } -void PreorderAST::RemoveNode(Node *N, Node *Parent) { - // The parent should be a BinaryNode. - assert(isa(Parent) && "Invalid parent"); - - auto *P = dyn_cast(Parent); - if (!P) { - SetError(); - return; - } +bool PreorderAST::CanCoalesceNode(BinaryNode *B, BinaryNode *P) { + if (!B || !isa(B) || + !P || !isa(P)) + return false; - // We will remove a BinaryNode only if its operator is commutative and + // 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. - if (auto *B = dyn_cast(N)) { - assert(B->IsOpCommutativeAndAssociative() && - "BinaryNode operator must be commutative and associative"); - if (!B->IsOpCommutativeAndAssociative()) { - SetError(); - return; - } + return B->Opc == P->Opc || + (B->Children.size() == 1 && + B->IsOpCommutativeAndAssociative() && + P->IsOpCommutativeAndAssociative()); +} + +void PreorderAST::CoalesceNode(BinaryNode *B, BinaryNode *P) { + if (!CanCoalesceNode(B, P)) { + assert(0 && "Attempting to coalesce invalid node"); + SetError(); + return; } // Remove the current node from the list of children of its parent. for (auto I = P->Children.begin(), E = P->Children.end(); I != E; ++I) { - if (*I == N) { + if (*I == B) { P->Children.erase(I); break; } } - if (auto *B = dyn_cast(N)) { - // Move all children of the current node to its parent. - for (auto *Child : B->Children) { - Child->Parent = P; - P->Children.push_back(Child); - } + // Move all children of the current node to its parent. + for (auto *Child : B->Children) { + Child->Parent = P; + P->Children.push_back(Child); } // Delete the current node. - delete N; + delete B; } void PreorderAST::Create(Expr *E, Node *Parent) { @@ -166,12 +167,8 @@ void PreorderAST::Coalesce(Node *N, bool &Changed) { if (!Parent) return; - // We can coalesce only if: - // 1. The parent has the same operator as the current node. - // 2. The current node is a BinaryNode with just one child (for example, as a - // result of constant folding). - if (Parent->Opc == B->Opc || B->Children.size() == 1) { - RemoveNode(B, Parent); + if (CanCoalesceNode(B, Parent)) { + CoalesceNode(B, Parent); Changed = true; } } From 75b0678eb1f13c45786c407219f986191e8d89cf Mon Sep 17 00:00:00 2001 From: Mandeep Singh Grang Date: Wed, 18 Nov 2020 12:20:04 -0800 Subject: [PATCH 4/6] Coalesce the single constant expr after constant folding --- clang/include/clang/AST/PreorderAST.h | 12 ++++-------- clang/lib/AST/PreorderAST.cpp | 27 ++++++++++++++++++++------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/clang/include/clang/AST/PreorderAST.h b/clang/include/clang/AST/PreorderAST.h index 63885a49e01c..b5c6ed4175f8 100644 --- a/clang/include/clang/AST/PreorderAST.h +++ b/clang/include/clang/AST/PreorderAST.h @@ -88,20 +88,16 @@ namespace clang { // @param[in] Parent is the parent of the node to be added. void AddNode(Node *N, Node *Parent); - // Coalesce the BinaryNode B with its parent P. This involves moving the + // Coalesce the BinaryNode B with its parent. This involves moving the // children (if any) of node B to its parent and then removing B. // @param[in] B is the current node. B should be a BinaryNode. - // @param[in] P is the parent of the node to be removed. P should be a - // BinaryNode. - void CoalesceNode(BinaryNode *B, BinaryNode *P); + void CoalesceNode(BinaryNode *B); // Determines if a BinaryNode could be coalesced into its parent. // @param[in] B is the current node. B should be a BinaryNode. - // @param[in] P is the parent of the node to be removed. P should be a - // BinaryNode. - // @return Return true if B can be coalesced into its parent P, false + // @return Return true if B can be coalesced into its parent, false // otherwise. - bool CanCoalesceNode(BinaryNode *B, BinaryNode *P); + bool CanCoalesceNode(BinaryNode *B); // Recursively coalesce binary nodes having the same commutative and // associative operator. diff --git a/clang/lib/AST/PreorderAST.cpp b/clang/lib/AST/PreorderAST.cpp index 8ade8922966c..a8296dc59abf 100644 --- a/clang/lib/AST/PreorderAST.cpp +++ b/clang/lib/AST/PreorderAST.cpp @@ -29,9 +29,12 @@ void PreorderAST::AddNode(Node *N, Node *Parent) { } } -bool PreorderAST::CanCoalesceNode(BinaryNode *B, BinaryNode *P) { - if (!B || !isa(B) || - !P || !isa(P)) +bool PreorderAST::CanCoalesceNode(BinaryNode *B) { + if (!B || !isa(B)) + return false; + + auto *P = dyn_cast(B->Parent); + if (!P) return false; // We can coalesce a BinaryNode if any one of the following conditions are @@ -47,13 +50,17 @@ bool PreorderAST::CanCoalesceNode(BinaryNode *B, BinaryNode *P) { P->IsOpCommutativeAndAssociative()); } -void PreorderAST::CoalesceNode(BinaryNode *B, BinaryNode *P) { - if (!CanCoalesceNode(B, P)) { +void PreorderAST::CoalesceNode(BinaryNode *B) { + if (!CanCoalesceNode(B)) { assert(0 && "Attempting to coalesce invalid node"); SetError(); return; } + // In the call to CanCoalesceNode we have made sure that the parent is a + // BinaryNode. So we can safely dyn_cast here. + auto *P = dyn_cast(B->Parent); + // Remove the current node from the list of children of its parent. for (auto I = P->Children.begin(), E = P->Children.end(); I != E; ++I) { @@ -167,8 +174,8 @@ void PreorderAST::Coalesce(Node *N, bool &Changed) { if (!Parent) return; - if (CanCoalesceNode(B, Parent)) { - CoalesceNode(B, Parent); + if (CanCoalesceNode(B)) { + CoalesceNode(B); Changed = true; } } @@ -321,6 +328,12 @@ void PreorderAST::ConstantFold(Node *N, bool &Changed) { // Add the constant folded expression to list of children of the current // BinaryNode. B->Children.push_back(new LeafExprNode(ConstFoldedExpr, B)); + + // If the constant folded expr is the only child of this BinaryNode we can + // coalesce the node. + if (B->Children.size() == 1 && CanCoalesceNode(B)) + CoalesceNode(B); + Changed = true; } From ca73be270035a56bdbbe80a6e3bb49f51a7f83db Mon Sep 17 00:00:00 2001 From: Mandeep Singh Grang Date: Thu, 19 Nov 2020 13:05:26 -0800 Subject: [PATCH 5/6] Simplify logic and add comments --- clang/lib/AST/PreorderAST.cpp | 31 ++++++++++++++++--------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/clang/lib/AST/PreorderAST.cpp b/clang/lib/AST/PreorderAST.cpp index a8296dc59abf..52608b844533 100644 --- a/clang/lib/AST/PreorderAST.cpp +++ b/clang/lib/AST/PreorderAST.cpp @@ -33,7 +33,7 @@ bool PreorderAST::CanCoalesceNode(BinaryNode *B) { if (!B || !isa(B)) return false; - auto *P = dyn_cast(B->Parent); + auto *P = dyn_cast_or_null(B->Parent); if (!P) return false; @@ -44,10 +44,19 @@ bool PreorderAST::CanCoalesceNode(BinaryNode *B) { // constant folding) and the parent and current operators are commutative and // associative. - return B->Opc == P->Opc || - (B->Children.size() == 1 && - B->IsOpCommutativeAndAssociative() && - P->IsOpCommutativeAndAssociative()); + // We can only coalesce if the operators on the current and parent nodes are + // commutative and associative. This is because after coalescing we later + // need to sort the nodes and if the operator is not commutative and + // associative then sorting would be incorrect. + if (!B->IsOpCommutativeAndAssociative() || + !P->IsOpCommutativeAndAssociative()) + return false; + + // We can coalesce in the following scenarios: + // 1. The current and parent nodes have the same operator OR + // 2. The current node is the only child of its operator node (maybe as a + // result of constant folding). + return B->Opc == P->Opc || B->Children.size() == 1; } void PreorderAST::CoalesceNode(BinaryNode *B) { @@ -57,8 +66,8 @@ void PreorderAST::CoalesceNode(BinaryNode *B) { return; } - // In the call to CanCoalesceNode we have made sure that the parent is a - // BinaryNode. So we can safely dyn_cast here. + // In the call to CanCoalesceNode above we have made sure that the parent is + // a BinaryNode. So we can safely dyn_cast here. auto *P = dyn_cast(B->Parent); // Remove the current node from the list of children of its parent. @@ -166,14 +175,6 @@ void PreorderAST::Coalesce(Node *N, bool &Changed) { if (isa(Child)) Coalesce(Child, Changed); - // We can only coalesce if the operator is commutative and associative. - if (!B->IsOpCommutativeAndAssociative()) - return; - - auto *Parent = dyn_cast_or_null(B->Parent); - if (!Parent) - return; - if (CanCoalesceNode(B)) { CoalesceNode(B); Changed = true; From c43a7ede568862092f94593fc2551ccb13841fa8 Mon Sep 17 00:00:00 2001 From: Mandeep Singh Grang Date: Thu, 19 Nov 2020 13:08:07 -0800 Subject: [PATCH 6/6] Fixed comments --- clang/lib/AST/PreorderAST.cpp | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/clang/lib/AST/PreorderAST.cpp b/clang/lib/AST/PreorderAST.cpp index 52608b844533..4a489d4bbcde 100644 --- a/clang/lib/AST/PreorderAST.cpp +++ b/clang/lib/AST/PreorderAST.cpp @@ -37,14 +37,7 @@ bool PreorderAST::CanCoalesceNode(BinaryNode *B) { if (!P) return false; - // 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. - - // We can only coalesce if the operators on the current and parent nodes are + // We can only coalesce if the operator of the current and parent node is // commutative and associative. This is because after coalescing we later // need to sort the nodes and if the operator is not commutative and // associative then sorting would be incorrect.