[TBAA] Only emit pointer tbaa metedata for record types. #116991
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fhahn
requested review from
zygoloid,
rjmccall,
AaronBallman and
efriedma-quic
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-codegen Author: Florian Hahn (fhahn) ChangesBe conservative if the type isn't a record type. Handling other types may Without this, we assign different tags to the accesses for p an q in the Full diff: https://github.com/llvm/llvm-project/pull/116991.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp b/clang/lib/CodeGen/CodeGenTBAA.cpp
index c31579e8323174..73aae6b0db8d90 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -230,6 +230,12 @@ llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type *Ty) {
->getString();
TyName = Name;
} else {
+ // Be conservative if the type isn't a record type. Handling other types
+ // may require stripping const-qualifiers inside the type, e.g.
+ // MemberPointerType.
+ if (!Ty->isRecordType())
+ return AnyPtr;
+
// For non-builtin types use the mangled name of the canonical type.
llvm::raw_svector_ostream TyOut(TyName);
MangleCtx->mangleCanonicalTypeName(QualType(Ty, 0), TyOut);
diff --git a/clang/test/CXX/drs/cwg158.cpp b/clang/test/CXX/drs/cwg158.cpp
index 9301c790297e9d..d20f715d637917 100644
--- a/clang/test/CXX/drs/cwg158.cpp
+++ b/clang/test/CXX/drs/cwg158.cpp
@@ -1,12 +1,14 @@
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++98 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++11 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++14 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++98 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++11 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++14 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -pointer-tbaa -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
// cwg158: yes
// CHECK-LABEL: define {{.*}} @_Z1f
const int *f(const int * const *p, int **q) {
+ // CHECK: load ptr, ptr %p.addr
// CHECK: load ptr, {{.*}}, !tbaa ![[INTPTR_TBAA:[^,]*]]
const int *x = *p;
// CHECK: store ptr null, {{.*}}, !tbaa ![[INTPTR_TBAA]]
@@ -18,10 +20,10 @@ struct A {};
// CHECK-LABEL: define {{.*}} @_Z1g
const int *(A::*const *g(const int *(A::* const **p)[3], int *(A::***q)[3]))[3] {
+ // CHECK: load ptr, ptr %p.addr
// CHECK: load ptr, {{.*}}, !tbaa ![[MEMPTR_TBAA:[^,]*]]
const int *(A::*const *x)[3] = *p;
// CHECK: store ptr null, {{.*}}, !tbaa ![[MEMPTR_TBAA]]
*q = 0;
return x;
}
-
|
|
@llvm/pr-subscribers-clang Author: Florian Hahn (fhahn) ChangesBe conservative if the type isn't a record type. Handling other types may Without this, we assign different tags to the accesses for p an q in the Full diff: https://github.com/llvm/llvm-project/pull/116991.diff 2 Files Affected:
diff --git a/clang/lib/CodeGen/CodeGenTBAA.cpp b/clang/lib/CodeGen/CodeGenTBAA.cpp
index c31579e8323174..73aae6b0db8d90 100644
--- a/clang/lib/CodeGen/CodeGenTBAA.cpp
+++ b/clang/lib/CodeGen/CodeGenTBAA.cpp
@@ -230,6 +230,12 @@ llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type *Ty) {
->getString();
TyName = Name;
} else {
+ // Be conservative if the type isn't a record type. Handling other types
+ // may require stripping const-qualifiers inside the type, e.g.
+ // MemberPointerType.
+ if (!Ty->isRecordType())
+ return AnyPtr;
+
// For non-builtin types use the mangled name of the canonical type.
llvm::raw_svector_ostream TyOut(TyName);
MangleCtx->mangleCanonicalTypeName(QualType(Ty, 0), TyOut);
diff --git a/clang/test/CXX/drs/cwg158.cpp b/clang/test/CXX/drs/cwg158.cpp
index 9301c790297e9d..d20f715d637917 100644
--- a/clang/test/CXX/drs/cwg158.cpp
+++ b/clang/test/CXX/drs/cwg158.cpp
@@ -1,12 +1,14 @@
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++98 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++11 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++14 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
-// RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++98 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++11 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++14 %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
+// RUN: %clang_cc1 -triple x86_64-linux -std=c++1z %s -O3 -pointer-tbaa -disable-llvm-passes -pedantic-errors -emit-llvm -o - | FileCheck --check-prefixes=CHECK %s
// cwg158: yes
// CHECK-LABEL: define {{.*}} @_Z1f
const int *f(const int * const *p, int **q) {
+ // CHECK: load ptr, ptr %p.addr
// CHECK: load ptr, {{.*}}, !tbaa ![[INTPTR_TBAA:[^,]*]]
const int *x = *p;
// CHECK: store ptr null, {{.*}}, !tbaa ![[INTPTR_TBAA]]
@@ -18,10 +20,10 @@ struct A {};
// CHECK-LABEL: define {{.*}} @_Z1g
const int *(A::*const *g(const int *(A::* const **p)[3], int *(A::***q)[3]))[3] {
+ // CHECK: load ptr, ptr %p.addr
// CHECK: load ptr, {{.*}}, !tbaa ![[MEMPTR_TBAA:[^,]*]]
const int *(A::*const *x)[3] = *p;
// CHECK: store ptr null, {{.*}}, !tbaa ![[MEMPTR_TBAA]]
*q = 0;
return x;
}
-
|
|
Hmm. We're talking here about C++'s type similarity rules, which come from C++ [conv.qual]p1:
Okay, so we have pointers, member pointers, and arrays, and the upshot is that we need to be ignoring qualifiers on the pointee/element type recursively in all of these. In the current code, we're falling short of that in two ways:
Your patch happens to conservatively fix both of these, but I think we should probably go ahead and fix the array issue now, and then maybe it's just member pointers that we need to bail out conservatively for. The right fix for the array issue is just that we need the loop to look through array structure at every step before it checks for another pointer type. |
fhahn
force-pushed
the
pointer-tbaa-record-type-only
branch
from
378ac8e to
f6adf00
Compare
rjmccall
requested changes
Nov 20, 2024
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| @@ -206,12 +206,14 @@ llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type *Ty) { | |||
| if (!CodeGenOpts.PointerTBAA) | |||
| return AnyPtr; | |||
| // Compute the depth of the pointer and generate a tag of the form "p<depth> | |||
| // <base type tag>". | |||
| // <base type tag>". Look through pointer and array types to determine the | |||
| // base type. | |||
There was a problem hiding this comment.
Suggestion as a replacement for the comment:
// C++ [basic.lval]p11 permits objects to accessed through an l-value
// of similar type. Two types are similar under C++ [conv.qual]p2
// if the decomposition of the types into pointers, member pointers,
// and arrays has the same structure when ignoring cv-qualifiers at
// each level of the decomposition. Meanwhile, C makes T(*)[] and
// T(*)[N] compatible, which would really complicate any attempt to
// distinguish pointers to arrays by their bounds. It's simpler, and
// much easier to explain to users, to simply treat all pointers to
// arrays as pointers to their element type for aliasing purposes.
// So when creating a TBAA tag for a pointer type, we recursively
// ignore both qualifiers and array types when decomposing
// the pointee type. The only meaningful remaining structure is the
// number of pointer types we encountered along the way, so we
// just produce the tag "p<depth> <base type tag>". If we do find a
// member pointer type, for now we just conservatively bail out
// with AnyPtr (below) rather than trying to create a tag that
// honors the similar-type rules while still distinguishing different
// kinds of member pointer.
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| Ty = Ty->getPointeeType().getTypePtr(); | ||
| } while (Ty->isPointerType()); | ||
| Ty = Ty->isPointerType() ? Ty->getPointeeType().getTypePtr() | ||
| : Ty->getArrayElementTypeNoTypeQual(); |
There was a problem hiding this comment.
You can just do Ty = Ty->getPointeeType()->getBaseElementTypeUnsafe();. The "unsafety" is just incorrectness about type qualifiers, which we're fine with here.
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| } while (Ty->isPointerType()); | ||
| Ty = Ty->isPointerType() ? Ty->getPointeeType().getTypePtr() | ||
| : Ty->getArrayElementTypeNoTypeQual(); | ||
| } while (Ty->isPointerType() || Ty->isArrayType()); | ||
| Ty = Context.getBaseElementType(QualType(Ty, 0)).getTypePtr(); |
| // Be conservative if the type a MemberPointerType. Those would require | ||
| // stripping const-qualifiers inside the type. | ||
| if (Ty->isMemberPointerType()) | ||
| return AnyPtr; |
There was a problem hiding this comment.
You know, you were probably right the first time to bail out for all non-record types — if we run into a vector type, or an ObjC pointer type, or something like that, it'd be better to fall back on AnyPtr. Please leave a comment saying that we're specifically required to do this for member pointers until we implement the similar-types rule, though.
There was a problem hiding this comment.
Restored and added, thanks!
fhahn
force-pushed
the
pointer-tbaa-record-type-only
branch
from
f6adf00 to
4e30502
Compare
rjmccall
reviewed
Nov 21, 2024
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| } while (Ty->isPointerType()); | ||
| Ty = Context.getBaseElementType(QualType(Ty, 0)).getTypePtr(); | ||
| Ty = Ty->getPointeeType()->getBaseElementTypeUnsafe(); | ||
| } while (Ty->isPointerType() || Ty->isArrayType()); |
There was a problem hiding this comment.
You don't need to check for array types anymore, getBaseElementTypeUnsafe() can never return an array type.
clang/lib/CodeGen/CodeGenTBAA.cpp
Outdated
| @@ -230,6 +243,12 @@ llvm::MDNode *CodeGenTBAA::getTypeInfoHelper(const Type *Ty) { | |||
| ->getString(); | |||
| TyName = Name; | |||
| } else { | |||
| // Be conservative if the type isn't a RecordType. We are specifically | |||
There was a problem hiding this comment.
Suggested change
| // Be conservative if the type isn't a RecordType. We are specifically | |
| // Be conservative if the type isn't a RecordType. We are specifically |
clang/test/CXX/drs/cwg158.cpp
Outdated
| } | ||
|
|
||
| // POINTER-TBAA: [[PTRARRAY_TBAA]] = !{[[PTRARRAY_TY:!.+]], [[PTRARRAY_TY]], i64 0} | ||
| // POINTER-TBAA: [[PTRARRAY_TY]] = !{!"p3 int", !4, i64 0} |
There was a problem hiding this comment.
Hmm. Why is this p3? I'd expect it to be p2 int: the l-value is of type int *(*)[], and we're ignoring all the array structure, so this should have the same tag as int **, i.e. p2 int, right?
Also, I assume the !4 is brittle. That's AnyPtr?
There was a problem hiding this comment.
The RUN line with -fpointer-tbaa was missing the POINTER-TBAA prefix, adjusted and fixed.
Be conservative if the type isn't a record type. Handling other types may require stripping const-qualifiers inside the type, e.g. MemberPointerType. Without this, we assign different tags to the accesses for p an q in the second test in cwg158.
fhahn
force-pushed
the
pointer-tbaa-record-type-only
branch
from
4e30502 to
2372e81
Compare
rjmccall
approved these changes
Nov 21, 2024
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Nov 21, 2024
Support for more precise TBAA metadata has been added a while ago (behind the -fpointer-tbaa flag). The more precise TBAA metadata allows treating accesses of different pointer types as no-alias. This helps to remove more redundant loads and stores in a number of workloads. Some highlights on the impact across llvm-test-suite's MultiSource, SPEC2006 & SPEC2017 include: * +2% more NoAlias results for memory access * +4% more loops vectorized * +3% more stores removed by DSE. This closes a relatively big gap to GCC, which has been supporting disambiguating based on pointer types for a long time. (https://clang.godbolt.org/z/K7Wbhrz4q) Pointer-TBAA support for pointers to builtin types has been added in llvm#76612. Support for user-defined types has been added in llvm#110569. There are 2 pending PRs with bug fixes for special cases uncovered during some of my testing: * llvm#116991 * llvm#116596
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Nov 21, 2024
Support for more precise TBAA metadata has been added a while ago (behind the -fpointer-tbaa flag). The more precise TBAA metadata allows treating accesses of different pointer types as no-alias. This helps to remove more redundant loads and stores in a number of workloads. Some highlights on the impact across llvm-test-suite's MultiSource, SPEC2006 & SPEC2017 include: * +2% more NoAlias results for memory access * +4% more loops vectorized * +3% more stores removed by DSE. This closes a relatively big gap to GCC, which has been supporting disambiguating based on pointer types for a long time. (https://clang.godbolt.org/z/K7Wbhrz4q) Pointer-TBAA support for pointers to builtin types has been added in llvm#76612. Support for user-defined types has been added in llvm#110569. There are 2 pending PRs with bug fixes for special cases uncovered during some of my testing: * llvm#116991 * llvm#116596
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Nov 22, 2024
Support for more precise TBAA metadata has been added a while ago (behind the -fpointer-tbaa flag). The more precise TBAA metadata allows treating accesses of different pointer types as no-alias. This helps to remove more redundant loads and stores in a number of workloads. Some highlights on the impact across llvm-test-suite's MultiSource, SPEC2006 & SPEC2017 include: * +2% more NoAlias results for memory access * +4% more loops vectorized * +3% more stores removed by DSE. This closes a relatively big gap to GCC, which has been supporting disambiguating based on pointer types for a long time. (https://clang.godbolt.org/z/K7Wbhrz4q) Pointer-TBAA support for pointers to builtin types has been added in llvm#76612. Support for user-defined types has been added in llvm#110569. There are 2 pending PRs with bug fixes for special cases uncovered during some of my testing: * llvm#116991 * llvm#116596
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Dec 3, 2024
Support for more precise TBAA metadata has been added a while ago (behind the -fpointer-tbaa flag). The more precise TBAA metadata allows treating accesses of different pointer types as no-alias. This helps to remove more redundant loads and stores in a number of workloads. Some highlights on the impact across llvm-test-suite's MultiSource, SPEC2006 & SPEC2017 include: * +2% more NoAlias results for memory access * +4% more loops vectorized * +3% more stores removed by DSE. This closes a relatively big gap to GCC, which has been supporting disambiguating based on pointer types for a long time. (https://clang.godbolt.org/z/K7Wbhrz4q) Pointer-TBAA support for pointers to builtin types has been added in llvm#76612. Support for user-defined types has been added in llvm#110569. There are 2 pending PRs with bug fixes for special cases uncovered during some of my testing: * llvm#116991 * llvm#116596
fhahn
added a commit
that referenced
this pull request
Dec 4, 2024
Support for more precise TBAA metadata has been added a while ago (behind the -fpointer-tbaa flag). The more precise TBAA metadata allows treating accesses of different pointer types as no-alias. This helps to remove more redundant loads and stores in a number of workloads. Some highlights on the impact across llvm-test-suite's MultiSource, SPEC2006 & SPEC2017 include: * +2% more NoAlias results for memory accesses * +3% more stores removed by DSE, * +4% more loops vectorized. This closes a relatively big gap to GCC, which has been supporting disambiguating based on pointer types for a long time. (https://clang.godbolt.org/z/K7Wbhrz4q) Pointer-TBAA support for pointers to builtin types has been added in #76612. Support for user-defined types has been added in #110569. There are 2 recent PRs with bug fixes for special cases uncovered during testing: * #116991 * #116596 PR: #117244
TIFitis
pushed a commit
to TIFitis/llvm-project
that referenced
this pull request
Dec 18, 2024
Support for more precise TBAA metadata has been added a while ago (behind the -fpointer-tbaa flag). The more precise TBAA metadata allows treating accesses of different pointer types as no-alias. This helps to remove more redundant loads and stores in a number of workloads. Some highlights on the impact across llvm-test-suite's MultiSource, SPEC2006 & SPEC2017 include: * +2% more NoAlias results for memory accesses * +3% more stores removed by DSE, * +4% more loops vectorized. This closes a relatively big gap to GCC, which has been supporting disambiguating based on pointer types for a long time. (https://clang.godbolt.org/z/K7Wbhrz4q) Pointer-TBAA support for pointers to builtin types has been added in llvm#76612. Support for user-defined types has been added in llvm#110569. There are 2 recent PRs with bug fixes for special cases uncovered during testing: * llvm#116991 * llvm#116596 PR: llvm#117244
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Jan 8, 2025
Be conservative if the type isn't a record type. Handling other types may require stripping const-qualifiers inside the type, e.g. MemberPointerType. Also look through array types same as through pointer types, to not pessimize arrays of pointers. Without this, we assign different tags to the accesses for p an q in the second test in cwg158. PR: llvm#116991 (cherry picked from commit decb874)
fhahn
added a commit
to fhahn/llvm-project
that referenced
this pull request
Jan 9, 2025
Be conservative if the type isn't a record type. Handling other types may require stripping const-qualifiers inside the type, e.g. MemberPointerType. Also look through array types same as through pointer types, to not pessimize arrays of pointers. Without this, we assign different tags to the accesses for p an q in the second test in cwg158. PR: llvm#116991 (cherry picked from commit decb874)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Be conservative if the type isn't a record type. Handling other types may
require stripping const-qualifiers inside the type, e.g. MemberPointerType.
Also look through array types same as through pointer types, to not pessimize
arrays of pointers.
Without this, we assign different tags to the accesses for p an q in the
second test in cwg158.