[PATCH] D31830: Emit invariant.group.barrier when using union field
This revision was automatically updated to reflect the committed changes.
Closed by commit rL304448: Emit invariant.group.barrier when using union field
(authored by Prazek).
Changed prior to commit:
https://reviews.llvm.org/D31830?vs=100977=101058#toc
Repository:
rL LLVM
https://reviews.llvm.org/D31830
Files:
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp
Index: cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp
+++ cfe/trunk/test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,119 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(>b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(>a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+struct Empty {};
+struct AnotherEmpty {
+ Empty e;
+};
+union NoVptrs {
+ int a;
+ AnotherEmpty empty;
+};
+void take(AnotherEmpty &);
+
+// CHECK-NEW-LABEL: noBarriers
+void noBarriers(NoVptrs ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ noVptrs.a += 42;
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR12AnotherEmpty(
+ take(noVptrs.empty);
+}
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
+
+struct VirtualInBase : HoldingVirtuals, Empty {
+};
+
+struct VirtualInVBase : virtual Empty, virtual HoldingVirtuals {
+};
+
+// It has vtable by virtual inheritance.
+struct VirtualInheritance : virtual Empty {
+};
+
+union U3 {
+ VirtualInBase v1;
+ VirtualInBase v2;
+ VirtualInheritance v3;
+ int z;
+};
+
+void take(VirtualInBase &);
+void take(VirtualInVBase &);
+void take(VirtualInheritance &);
+
+void UnionsBarrier3(U3 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR13VirtualInBase(
+ take(u.v1);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR13VirtualInBase(
+ take(u.v2);
+
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR18VirtualInheritance(
+ take(u.v3);
+}
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: cfe/trunk/lib/CodeGen/CGExpr.cpp
===
--- cfe/trunk/lib/CodeGen/CGExpr.cpp
+++ cfe/trunk/lib/CodeGen/CGExpr.cpp
@@ -3530,6 +3530,25 @@
return CGF.Builder.CreateStructGEP(base, idx, offset, field->getName());
}
+static bool hasAnyVptr(const QualType Type, const ASTContext ) {
+ const auto *RD = Type.getTypePtr()->getAsCXXRecordDecl();
+ if (!RD)
+return false;
+
+ if (RD->isDynamicClass())
+return true;
+
+ for (const auto : RD->bases())
+if (hasAnyVptr(Base.getType(), Context))
+ return true;
+
+ for (const FieldDecl *Field : RD->fields())
+if (hasAnyVptr(Field->getType(), Context))
+ return true;
+
+ return false;
+}
+
LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
LValueBaseInfo
[PATCH] D31830: Emit invariant.group.barrier when using union field
rjmccall accepted this revision. rjmccall added a comment. Thanks, LGTM. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek updated this revision to Diff 100977.
Prazek marked an inline comment as done.
Prazek added a comment.
Extra test
https://reviews.llvm.org/D31830
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGenCXX/strict-vtable-pointers.cpp
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,119 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(>b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(>a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+struct Empty {};
+struct AnotherEmpty {
+ Empty e;
+};
+union NoVptrs {
+ int a;
+ AnotherEmpty empty;
+};
+void take(AnotherEmpty &);
+
+// CHECK-NEW-LABEL: noBarriers
+void noBarriers(NoVptrs ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ noVptrs.a += 42;
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR12AnotherEmpty(
+ take(noVptrs.empty);
+}
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
+
+struct VirtualInBase : HoldingVirtuals, Empty {
+};
+
+struct VirtualInVBase : virtual Empty, virtual HoldingVirtuals {
+};
+
+// It has vtable by virtual inheritance.
+struct VirtualInheritance : virtual Empty {
+};
+
+union U3 {
+ VirtualInBase v1;
+ VirtualInBase v2;
+ VirtualInheritance v3;
+ int z;
+};
+
+void take(VirtualInBase &);
+void take(VirtualInVBase &);
+void take(VirtualInheritance &);
+
+void UnionsBarrier3(U3 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR13VirtualInBase(
+ take(u.v1);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR13VirtualInBase(
+ take(u.v2);
+
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR18VirtualInheritance(
+ take(u.v3);
+}
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3530,6 +3530,25 @@
return CGF.Builder.CreateStructGEP(base, idx, offset, field->getName());
}
+static bool hasAnyVptr(const QualType Type, const ASTContext ) {
+ const auto *RD = Type.getTypePtr()->getAsCXXRecordDecl();
+ if (!RD)
+return false;
+
+ if (RD->isDynamicClass())
+return true;
+
+ for (const auto : RD->bases())
+if (hasAnyVptr(Base.getType(), Context))
+ return true;
+
+ for (const FieldDecl *Field : RD->fields())
+if (hasAnyVptr(Field->getType(), Context))
+ return true;
+
+ return false;
+}
+
LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
LValueBaseInfo BaseInfo = base.getBaseInfo();
@@ -3572,6 +3591,14 @@
assert(!type->isReferenceType() && "union has reference member");
// TODO: handle path-aware TBAA for union.
TBAAPath = false;
+
+const auto FieldType =
[PATCH] D31830: Emit invariant.group.barrier when using union field
rjmccall added inline comments.
Comment at: test/CodeGenCXX/strict-vtable-pointers.cpp:263
+
+struct VirtualInVBase : virtual Empty, HoldingVirtuals {
+};
"virtual" applies to an individual base, not to all following bases. You need
"virtual HoldingVirtuals" here.
Although, come to think of it, having a virtual base should already ensure that
the class has a v-table, even if it has no other reason for one. (You should
add a test for that!)
https://reviews.llvm.org/D31830
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek updated this revision to Diff 100621.
Prazek added a comment.
Add test
https://reviews.llvm.org/D31830
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGenCXX/strict-vtable-pointers.cpp
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,112 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(>b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(>a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+struct Empty {};
+struct AnotherEmpty {
+ Empty e;
+};
+union NoVptrs {
+ int a;
+ AnotherEmpty empty;
+};
+void take(AnotherEmpty &);
+
+// CHECK-NEW-LABEL: noBarriers
+void noBarriers(NoVptrs ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ noVptrs.a += 42;
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR12AnotherEmpty(
+ take(noVptrs.empty);
+}
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
+
+struct VirtualInBase : HoldingVirtuals, Empty {
+};
+
+struct VirtualInVBase : virtual Empty, HoldingVirtuals {
+};
+
+union U3 {
+ VirtualInBase v1;
+ VirtualInBase v2;
+ int z;
+};
+
+void take(VirtualInBase &);
+void take(VirtualInVBase &);
+
+void UnionsBarrier3(U3 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR13VirtualInBase(
+ take(u.v1);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR13VirtualInBase(
+ take(u.v2);
+}
+
+
+
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3528,6 +3528,25 @@
return CGF.Builder.CreateStructGEP(base, idx, offset, field->getName());
}
+static bool hasAnyVptr(const QualType Type, const ASTContext ) {
+ const auto *RD = Type.getTypePtr()->getAsCXXRecordDecl();
+ if (!RD)
+return false;
+
+ if (RD->isDynamicClass())
+return true;
+
+ for (const auto : RD->bases())
+if (hasAnyVptr(Base.getType(), Context))
+ return true;
+
+ for (const FieldDecl *Field : RD->fields())
+if (hasAnyVptr(Field->getType(), Context))
+ return true;
+
+ return false;
+}
+
LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
LValueBaseInfo BaseInfo = base.getBaseInfo();
@@ -3569,6 +3588,14 @@
assert(!type->isReferenceType() && "union has reference member");
// TODO: handle path-aware TBAA for union.
TBAAPath = false;
+
+const auto FieldType = field->getType();
+if (CGM.getCodeGenOpts().StrictVTablePointers &&
+hasAnyVptr(FieldType, getContext()))
+ // Because unions can easily skip invariant.barriers, we need to add
+ // a barrier every time CXXRecord field with vptr is referenced.
+ addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+
[PATCH] D31830: Emit invariant.group.barrier when using union field
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Looks great, thanks. One request for additional tests, but feel free to commit when that's ready. Comment at: test/CodeGenCXX/strict-vtable-pointers.cpp:258 + take(u.h); +} Please include test cases involving classes that only have a virtual pointer in a field of a base class — e.g. something that inherits from HoldingVirtuals. And be sure to cover both normal and virtual inheritance. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek marked 2 inline comments as done. Prazek added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3530 + return false; +} + rjmccall wrote: > You need to recurse into base classes (to check their fields), and if you > write this to take a QualType you won't have to eagerly extract RD below. Cool, thanks for the tip https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek updated this revision to Diff 100533.
Prazek added a comment.
- changed to QualType, now it is much cleaner
https://reviews.llvm.org/D31830
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGenCXX/strict-vtable-pointers.cpp
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,82 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(>b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(>a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+struct Empty {};
+struct AnotherEmpty {
+ Empty e;
+};
+union NoVptrs {
+ int a;
+ AnotherEmpty empty;
+};
+void take(AnotherEmpty &);
+
+// CHECK-NEW-LABEL: noBarriers
+void noBarriers(NoVptrs ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ noVptrs.a += 42;
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR12AnotherEmpty(
+ take(noVptrs.empty);
+}
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3528,6 +3528,25 @@
return CGF.Builder.CreateStructGEP(base, idx, offset, field->getName());
}
+static bool hasAnyVptr(const QualType Type, const ASTContext ) {
+ const auto *RD = Type.getTypePtr()->getAsCXXRecordDecl();
+ if (!RD)
+return false;
+
+ if (RD->isDynamicClass())
+return true;
+
+ for (const auto : RD->bases())
+if (hasAnyVptr(Base.getType(), Context))
+ return true;
+
+ for (const FieldDecl *Field : RD->fields())
+if (hasAnyVptr(Field->getType(), Context))
+ return true;
+
+ return false;
+}
+
LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
LValueBaseInfo BaseInfo = base.getBaseInfo();
@@ -3569,6 +3588,14 @@
assert(!type->isReferenceType() && "union has reference member");
// TODO: handle path-aware TBAA for union.
TBAAPath = false;
+
+const auto FieldType = field->getType();
+if (CGM.getCodeGenOpts().StrictVTablePointers &&
+hasAnyVptr(FieldType, getContext()))
+ // Because unions can easily skip invariant.barriers, we need to add
+ // a barrier every time CXXRecord field with vptr is referenced.
+ addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+ addr.getAlignment());
} else {
// For structs, we GEP to the field that the record layout suggests.
addr = emitAddrOfFieldStorage(*this, addr, field);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), Prazek wrote: > rjmccall wrote: > > Prazek wrote: > > > rjmccall wrote: > > > > Prazek wrote: > > > > > rjmccall wrote: > > > > > > Checking for v-table pointers recursively isn't really that > > > > > > difficult, but if you really don't want to do that, please at least > > > > > > check for non-POD-ness or something so that this isn't kicking in > > > > > > for literally every struct type. > > > > > ok, I am also planning to fix this in the later patch, because the > > > > > same problem arise when comparing 2 pointers to dynamic classe. > > > > > I would like to have a bit in CXXRecordDecl to remember if it has any > > > > > vptr inside that would calculate durring the construction. > > > > > My biggest fear is that if I won't cache it then it will be very slow. > > > > We could repeat this work from scratch on every single union field > > > > access done by IRGen and it would still be way, way faster than doing > > > > anything extra on every record definition in the program. The latter > > > > is done orders of magnitude more frequently than the former. > > > This is a good point, and actually I don't need to check if class holds > > > any vptrs for the example I refered (comparing 2 pointers). > > > Hovewer I still need to do it when someone casts pointer to class holding > > > vptrs to something that doesn't hold vptrs, and any pointer casts are > > > much more frequent than Record declarations. > > What are you planning to do if someone casts to a pointer to incomplete > > type? > > > > I am concerned that what these problems are really telling us is that this > > representation is not very good. > In this case we also need to add the barrier. I might be wrong, but I think > this in general should be pretty rare and even more rare if it would limit > the devirtualization. > For example cases like: > > Derived *d; > (Base*)d; > > will not limit devirtualization if Derived is complete type (because base > will be also). > > The other thing is that with my recent patches, adding more barriers will not > limit any memory optimizations. You don't need the .getTypePtr() here. Comment at: lib/CodeGen/CGExpr.cpp:3530 + return false; +} + You need to recurse into base classes (to check their fields), and if you write this to take a QualType you won't have to eagerly extract RD below. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek added a reviewer: mehdi_amini. Prazek added a comment. Ping https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek updated this revision to Diff 96312.
Prazek added a comment.
- Inserting barrier with -O0
https://reviews.llvm.org/D31830
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGenCXX/strict-vtable-pointers.cpp
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,82 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(>b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(>a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+struct Empty {};
+struct AnotherEmpty {
+ Empty e;
+};
+union NoVptrs {
+ int a;
+ AnotherEmpty empty;
+};
+void take(AnotherEmpty &);
+
+// CHECK-NEW-LABEL: noBarriers
+void noBarriers(NoVptrs ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ noVptrs.a += 42;
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR12AnotherEmpty(
+ take(noVptrs.empty);
+}
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3513,6 +3513,22 @@
return CGF.Builder.CreateStructGEP(base, idx, offset, field->getName());
}
+static bool hasAnyVptr(const CXXRecordDecl *RD, const ASTContext ) {
+ if (RD->isDynamicClass())
+return true;
+
+ for (const FieldDecl *Field : RD->fields()) {
+QualType FieldType = Context.getBaseElementType(Field->getType());
+const RecordType *RT = FieldType->getAs();
+if (!RT)
+ continue;
+auto *FieldClassDecl = cast(RT->getDecl());
+if (hasAnyVptr(FieldClassDecl, Context))
+ return true;
+ }
+ return false;
+}
+
LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
AlignmentSource fieldAlignSource =
@@ -3552,6 +3568,14 @@
assert(!type->isReferenceType() && "union has reference member");
// TODO: handle path-aware TBAA for union.
TBAAPath = false;
+
+const auto *RD = field->getType().getTypePtr()->getAsCXXRecordDecl();
+if (CGM.getCodeGenOpts().StrictVTablePointers &&
+RD && hasAnyVptr(RD, getContext()))
+ // Because unions can easily skip invariant.barriers, we need to add
+ // a barrier every time CXXRecord field with vptr is referenced.
+ addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+ addr.getAlignment());
} else {
// For structs, we GEP to the field that the record layout suggests.
addr = emitAddrOfFieldStorage(*this, addr, field);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek updated this revision to Diff 96255.
Prazek added a comment.
- format
https://reviews.llvm.org/D31830
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGenCXX/strict-vtable-pointers.cpp
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,82 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(>b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(>a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+struct Empty {};
+struct AnotherEmpty {
+ Empty e;
+};
+union NoVptrs {
+ int a;
+ AnotherEmpty empty;
+};
+void take(AnotherEmpty &);
+
+// CHECK-NEW-LABEL: noBarriers
+void noBarriers(NoVptrs ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ noVptrs.a += 42;
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR12AnotherEmpty(
+ take(noVptrs.empty);
+}
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3513,6 +3513,22 @@
return CGF.Builder.CreateStructGEP(base, idx, offset, field->getName());
}
+static bool hasAnyVptr(const CXXRecordDecl *RD, const ASTContext ) {
+ if (RD->isDynamicClass())
+return true;
+
+ for (const FieldDecl *Field : RD->fields()) {
+QualType FieldType = Context.getBaseElementType(Field->getType());
+const RecordType *RT = FieldType->getAs();
+if (!RT)
+ continue;
+auto *FieldClassDecl = cast(RT->getDecl());
+if (hasAnyVptr(FieldClassDecl, Context))
+ return true;
+ }
+ return false;
+}
+
LValue CodeGenFunction::EmitLValueForField(LValue base,
const FieldDecl *field) {
AlignmentSource fieldAlignSource =
@@ -3552,6 +3568,15 @@
assert(!type->isReferenceType() && "union has reference member");
// TODO: handle path-aware TBAA for union.
TBAAPath = false;
+
+const auto *RD = field->getType().getTypePtr()->getAsCXXRecordDecl();
+if (CGM.getCodeGenOpts().StrictVTablePointers &&
+CGM.getCodeGenOpts().OptimizationLevel > 0 && RD &&
+hasAnyVptr(RD, getContext()))
+ // Because unions can easily skip invariant.barriers, we need to add
+ // a barrier every time CXXRecord field with vptr is referenced.
+ addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+ addr.getAlignment());
} else {
// For structs, we GEP to the field that the record layout suggests.
addr = emitAddrOfFieldStorage(*this, addr, field);
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek marked 6 inline comments as done. Prazek added a comment. For now I will check if it has any vptrs. It will be probably soon stored in the CXXRecordDecl because it will be used much more frequently when generating barriers for pointer casts. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), rjmccall wrote: > Prazek wrote: > > rjmccall wrote: > > > Prazek wrote: > > > > rjmccall wrote: > > > > > Checking for v-table pointers recursively isn't really that > > > > > difficult, but if you really don't want to do that, please at least > > > > > check for non-POD-ness or something so that this isn't kicking in for > > > > > literally every struct type. > > > > ok, I am also planning to fix this in the later patch, because the same > > > > problem arise when comparing 2 pointers to dynamic classe. > > > > I would like to have a bit in CXXRecordDecl to remember if it has any > > > > vptr inside that would calculate durring the construction. > > > > My biggest fear is that if I won't cache it then it will be very slow. > > > We could repeat this work from scratch on every single union field access > > > done by IRGen and it would still be way, way faster than doing anything > > > extra on every record definition in the program. The latter is done > > > orders of magnitude more frequently than the former. > > This is a good point, and actually I don't need to check if class holds any > > vptrs for the example I refered (comparing 2 pointers). > > Hovewer I still need to do it when someone casts pointer to class holding > > vptrs to something that doesn't hold vptrs, and any pointer casts are much > > more frequent than Record declarations. > What are you planning to do if someone casts to a pointer to incomplete type? > > I am concerned that what these problems are really telling us is that this > representation is not very good. In this case we also need to add the barrier. I might be wrong, but I think this in general should be pretty rare and even more rare if it would limit the devirtualization. For example cases like: Derived *d; (Base*)d; will not limit devirtualization if Derived is complete type (because base will be also). The other thing is that with my recent patches, adding more barriers will not limit any memory optimizations. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), Prazek wrote: > rjmccall wrote: > > Prazek wrote: > > > rjmccall wrote: > > > > Checking for v-table pointers recursively isn't really that difficult, > > > > but if you really don't want to do that, please at least check for > > > > non-POD-ness or something so that this isn't kicking in for literally > > > > every struct type. > > > ok, I am also planning to fix this in the later patch, because the same > > > problem arise when comparing 2 pointers to dynamic classe. > > > I would like to have a bit in CXXRecordDecl to remember if it has any > > > vptr inside that would calculate durring the construction. > > > My biggest fear is that if I won't cache it then it will be very slow. > > We could repeat this work from scratch on every single union field access > > done by IRGen and it would still be way, way faster than doing anything > > extra on every record definition in the program. The latter is done orders > > of magnitude more frequently than the former. > This is a good point, and actually I don't need to check if class holds any > vptrs for the example I refered (comparing 2 pointers). > Hovewer I still need to do it when someone casts pointer to class holding > vptrs to something that doesn't hold vptrs, and any pointer casts are much > more frequent than Record declarations. What are you planning to do if someone casts to a pointer to incomplete type? I am concerned that what these problems are really telling us is that this representation is not very good. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), rjmccall wrote: > Prazek wrote: > > rjmccall wrote: > > > Checking for v-table pointers recursively isn't really that difficult, > > > but if you really don't want to do that, please at least check for > > > non-POD-ness or something so that this isn't kicking in for literally > > > every struct type. > > ok, I am also planning to fix this in the later patch, because the same > > problem arise when comparing 2 pointers to dynamic classe. > > I would like to have a bit in CXXRecordDecl to remember if it has any vptr > > inside that would calculate durring the construction. > > My biggest fear is that if I won't cache it then it will be very slow. > We could repeat this work from scratch on every single union field access > done by IRGen and it would still be way, way faster than doing anything extra > on every record definition in the program. The latter is done orders of > magnitude more frequently than the former. This is a good point, and actually I don't need to check if class holds any vptrs for the example I refered (comparing 2 pointers). Hovewer I still need to do it when someone casts pointer to class holding vptrs to something that doesn't hold vptrs, and any pointer casts are much more frequent than Record declarations. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), Prazek wrote: > rjmccall wrote: > > Checking for v-table pointers recursively isn't really that difficult, but > > if you really don't want to do that, please at least check for non-POD-ness > > or something so that this isn't kicking in for literally every struct type. > ok, I am also planning to fix this in the later patch, because the same > problem arise when comparing 2 pointers to dynamic classe. > I would like to have a bit in CXXRecordDecl to remember if it has any vptr > inside that would calculate durring the construction. > My biggest fear is that if I won't cache it then it will be very slow. We could repeat this work from scratch on every single union field access done by IRGen and it would still be way, way faster than doing anything extra on every record definition in the program. The latter is done orders of magnitude more frequently than the former. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), rjmccall wrote: > Checking for v-table pointers recursively isn't really that difficult, but if > you really don't want to do that, please at least check for non-POD-ness or > something so that this isn't kicking in for literally every struct type. ok, I am also planning to fix this in the later patch, because the same problem arise when comparing 2 pointers to dynamic classe. I would like to have a bit in CXXRecordDecl to remember if it has any vptr inside that would calculate durring the construction. My biggest fear is that if I won't cache it then it will be very slow. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
rjmccall added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:3517 +CGM.getCodeGenOpts().StrictVTablePointers && +CGM.getCodeGenOpts().OptimizationLevel > 0) + addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()), Checking for v-table pointers recursively isn't really that difficult, but if you really don't want to do that, please at least check for non-POD-ness or something so that this isn't kicking in for literally every struct type. https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek added a comment. ping https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek added a comment. ping https://reviews.llvm.org/D31830 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D31830: Emit invariant.group.barrier when using union field
Prazek created this revision.
We need to emit barrier if the union field
is CXXRecordDecl because it might have vptrs. The testcode
was wrongly devirtualized. It also proves that having different
groups for different dynamic types is not sufficient.
https://reviews.llvm.org/D31830
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGenCXX/strict-vtable-pointers.cpp
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10
-fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10
-fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,62 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
+ // CHECK-NEW: call void @_Z9changeToBP1U(
+ changeToB(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z2g2P1A(%struct.A*
+ g2(>b);
+ // CHECK-NEW: call void @_Z9changeToAP1U(%union.U* %6)
+ changeToA(u);
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // call void @_Z2g2P1A(%struct.A* %a)
+ g2(>a);
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+}
+
+struct HoldingVirtuals {
+ A a;
+};
+
+union U2 {
+ HoldingVirtuals h;
+ int z;
+};
+void take(HoldingVirtuals &);
+
+// CHECK-NEW-LABEL: define void @_Z15UnionsBarriers2R2U2
+void UnionsBarriers2(U2 ) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: 42
+ u.z += 42;
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: call void @_Z4takeR15HoldingVirtuals(
+ take(u.h);
+}
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -3508,6 +3508,15 @@
assert(!type->isReferenceType() && "union has reference member");
// TODO: handle path-aware TBAA for union.
TBAAPath = false;
+// Because unions can easily skip invariant.barriers, we need to add
+// a barrier every time CXXRecord field is referenced. We don't check if
+// it is dynamic type because it might have dynamic member, so we would
+// have to check it recursively.
+if (field->getType().getTypePtr()->getAsCXXRecordDecl() &&
+CGM.getCodeGenOpts().StrictVTablePointers &&
+CGM.getCodeGenOpts().OptimizationLevel > 0)
+ addr = Address(Builder.CreateInvariantGroupBarrier(addr.getPointer()),
+ addr.getAlignment());
} else {
// For structs, we GEP to the field that the record layout suggests.
addr = emitAddrOfFieldStorage(*this, addr, field);
Index: test/CodeGenCXX/strict-vtable-pointers.cpp
===
--- test/CodeGenCXX/strict-vtable-pointers.cpp
+++ test/CodeGenCXX/strict-vtable-pointers.cpp
@@ -1,4 +1,4 @@
-// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -disable-llvm-passes -O2 -emit-llvm -o %t.ll
+// RUN: %clang_cc1 %s -I%S -triple=x86_64-apple-darwin10 -fstrict-vtable-pointers -std=c++11 -disable-llvm-passes -O2 -emit-llvm -o %t.ll
// RUN: FileCheck --check-prefix=CHECK-CTORS %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-NEW %s < %t.ll
// RUN: FileCheck --check-prefix=CHECK-DTORS %s < %t.ll
@@ -180,6 +180,62 @@
// CHECK-CTORS-NOT: @llvm.invariant.group.barrier(
// CHECK-CTORS-LABEL: {{^}}}
+struct A {
+ virtual void foo();
+};
+struct B : A {
+ virtual void foo();
+};
+
+union U {
+ A a;
+ B b;
+};
+
+void changeToB(U *u);
+void changeToA(U *u);
+
+void g2(A *a) {
+ a->foo();
+}
+// We have to guard access to union fields with invariant.group, because
+// it is very easy to skip the barrier with unions. In this example the inlined
+// g2 will produce loads with the same !invariant.group metadata, and
+// u->a and u->b would use the same pointer.
+// CHECK-NEW-LABEL: define void @_Z14UnionsBarriersP1U
+void UnionsBarriers(U *u) {
