[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons
Prazek added a comment. In https://reviews.llvm.org/D32378#741989, @hubert.reinterpretcast wrote: > Has it been discussed whether this is something to be addressed in the > optimizer as opposed to the front-end? The example that you showed is excellent. I didn't know that LLVM does the transformation with pointers and it clearly shows that we need the different approach. I got following idea how to solve it in the middle end - drop all invariant.group metadata when we replace dominated uses based on the comparison. This works for simple cases, but it doesn't when the changed pointer is passed to a not inlined function, or returned. The solution I see that does not satisfy me, is to put barriers between passed/returned pointer. I will bring it to mailing list https://reviews.llvm.org/D32378 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons
hubert.reinterpretcast added a comment.
Has it been discussed whether this is something to be addressed in the
optimizer as opposed to the front-end?
Comment at: lib/CodeGen/CGExprScalar.cpp:3069
+ !isa(RHS)) {
+// Based on comparisons of pointers to dynamic objects, the optimizer
+// can replace one pointer with another. This might result in
Consider:
```
extern "C" int printf(const char *, ...);
void *operator new(decltype(sizeof 0), void *);
struct A {
virtual void foo() { printf("%s\n", __PRETTY_FUNCTION__); }
int m;
int *zip() { return &m; }
};
struct B : A {
virtual void foo() { printf("%s\n", __PRETTY_FUNCTION__); }
};
int main(void) {
A *ap = new A;
ap->foo();
int *const apz = ap->zip();
B *bp = new (ap) B;
if (apz == bp->zip()) {
bp->foo();
}
}
```
https://reviews.llvm.org/D32378
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons
Prazek updated this revision to Diff 96371.
Prazek added a comment.
Don't add barrier if compared with nullptr. With this it reduces added
barriers to compares by half. Note that we will transform barrier(null) ->
barrier
in llvm
https://reviews.llvm.org/D32378
Files:
lib/CodeGen/CGExprScalar.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
@@ -257,6 +257,72 @@
take(u.h);
}
+// CHECK-NEW-LABEL: define void @_Z7comparev()
+void compare() {
+ A *a = new A;
+ a->foo();
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ A *b = new (a) B;
+
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp eq %struct.A* %[[a2]], %[[b2]]
+ if (a == b)
+b->foo();
+}
+
+// CHECK-NEW-LABEL: compare2
+bool compare2(A *a, A *a2) {
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp ult %struct.A* %[[a2]], %[[b2]]
+ return a < a2;
+}
+// CHECK-NEW-LABEL: compareIntPointers
+bool compareIntPointers(int *a, int *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier
+ return a == b;
+}
+
+struct HoldingOtherVirtuals {
+ B b;
+};
+
+// There is no need to add barriers for comparision of pointer to classes
+// that are not dynamic.
+// CHECK-NEW-LABEL: compare5
+bool compare5(HoldingOtherVirtuals *a, HoldingOtherVirtuals *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier
+ return a == b;
+}
+// CHECK-NEW-LABEL: compareNull
+bool compareNull(A *a) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier
+
+ if (a != nullptr)
+return false;
+ if (!a)
+return false;
+ return a == nullptr;
+}
+
+struct X;
+// We have to also introduce the barriers if comparing pointers to incomplete
+// objects
+// CHECK-NEW-LABEL: define zeroext i1 @_Z8compare4P1XS0_
+bool compare4(X *x, X *x2) {
+ // CHECK-NEW: %[[x:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[xp:.*]] = bitcast i8* %[[x]] to %struct.X*
+ // CHECK-NEW: %[[x2:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[x2p:.*]] = bitcast i8* %[[x2]] to %struct.X*
+ // CHECK-NEW: %cmp = icmp eq %struct.X* %[[xp]], %[[x2p]]
+ return x == x2;
+}
+
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
// CHECK-DTORS-NOT: call i8* @llvm.invariant.group.barrier(
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1698,7 +1698,7 @@
}
case CK_IntToOCLSampler:
-return CGF.CGM.createOpenCLIntToSamplerConversion(E, CGF);
+return CGF.CGM.createOpenCLIntToSamplerConversion(E, CGF);
} // end of switch
@@ -3062,8 +3062,22 @@
Result = Builder.CreateFCmp(FCmpOpc, LHS, RHS, "cmp");
} else if (LHSTy->hasSignedIntegerRepresentation()) {
Result = Builder.CreateICmp(SICmpOpc, LHS, RHS, "cmp");
-} else {
- // Unsigned integers and pointers.
+} else { // Unsigned integers and pointers.
+ if (CGF.CGM.getCodeGenOpts().StrictVTablePointers &&
+ !isa(LHS) &&
+ !isa(RHS)) {
+// Based on comparisons of pointers to dynamic objects, the optimizer
+// can replace one pointer with another. This might result in
+// replacing pointer after barrier to pointer before barrier,
+// resulting in invalid devirtualization. Compare with null is safe.
+
+if (auto *RD = LHSTy->getPointeeCXXRecordDecl())
+ if (!RD->isCompleteDefinition() || RD->isDynamicClass())
+LHS = Builder.CreateInvariantGroupBarrier(LHS);
+if (auto *RD = RHSTy->getPointeeCXXRecordDecl())
+ if (!RD->isCompleteDefinition() || RD->isDynamicClass())
+RHS = Builder.CreateInvariantGroupBarrier(RHS);
+ }
Result = Builder.CreateICmp(UICmpOpc, LHS, RHS, "cmp");
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons
Prazek added a comment. This is actually good catch, we also need to do it when inserting barrier in placement new. I think that for the ctors and dtors we can do it only with optimizations enabled, because if optimizations are not enabled then ctors and dtors won't have the invariant.group in stores. https://reviews.llvm.org/D32378 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons
Prazek updated this revision to Diff 96311.
Prazek marked an inline comment as done.
Prazek added a comment.
Addressing Richard's comment
https://reviews.llvm.org/D32378
Files:
lib/CodeGen/CGExprScalar.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
@@ -257,6 +257,62 @@
take(u.h);
}
+// CHECK-NEW-LABEL: define void @_Z7comparev()
+void compare() {
+ A *a = new A;
+ a->foo();
+ // CHECK-NEW: call i8* @llvm.invariant.group.barrier(i8*
+ A *b = new (a) B;
+
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp eq %struct.A* %[[a2]], %[[b2]]
+ if (a == b)
+b->foo();
+}
+
+// CHECK-NEW-LABEL: compare2
+bool compare2(A *a, A *a2) {
+ // CHECK-NEW: %[[a:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[a2:.*]] = bitcast i8* %[[a]] to %struct.A*
+ // CHECK-NEW: %[[b:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[b2:.*]] = bitcast i8* %[[b]] to %struct.A*
+ // CHECK-NEW: %cmp = icmp ult %struct.A* %[[a2]], %[[b2]]
+ return a < a2;
+}
+// CHECK-NEW-LABEL: compareIntPointers
+bool compareIntPointers(int *a, int *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier
+ return a == b;
+}
+
+struct HoldingOtherVirtuals {
+ B b;
+};
+
+// There is no need to add barriers for comparision of pointer to classes
+// that are not dynamic.
+// CHECK-NEW-LABEL: compare5
+bool compare5(HoldingOtherVirtuals *a, HoldingOtherVirtuals *b) {
+ // CHECK-NEW-NOT: call i8* @llvm.invariant.group.barrier
+ return a == b;
+}
+
+struct X;
+// We have to also introduce the barriers if comparing pointers to incomplete
+// objects
+// CHECK-NEW-LABEL: define zeroext i1 @_Z8compare4P1XS0_
+bool compare4(X *x, X *x2) {
+ // CHECK-NEW: %[[x:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[xp:.*]] = bitcast i8* %[[x]] to %struct.X*
+ // CHECK-NEW: %[[x2:.*]] = call i8* @llvm.invariant.group.barrier(i8*
+ // CHECK-NEW: %[[x2p:.*]] = bitcast i8* %[[x2]] to %struct.X*
+ // CHECK-NEW: %cmp = icmp eq %struct.X* %[[xp]], %[[x2p]]
+ return x == x2;
+}
+
/** DTORS **/
// CHECK-DTORS-LABEL: define linkonce_odr void @_ZN10StaticBaseD2Ev(
// CHECK-DTORS-NOT: call i8* @llvm.invariant.group.barrier(
Index: lib/CodeGen/CGExprScalar.cpp
===
--- lib/CodeGen/CGExprScalar.cpp
+++ lib/CodeGen/CGExprScalar.cpp
@@ -1698,7 +1698,7 @@
}
case CK_IntToOCLSampler:
-return CGF.CGM.createOpenCLIntToSamplerConversion(E, CGF);
+return CGF.CGM.createOpenCLIntToSamplerConversion(E, CGF);
} // end of switch
@@ -3062,8 +3062,19 @@
Result = Builder.CreateFCmp(FCmpOpc, LHS, RHS, "cmp");
} else if (LHSTy->hasSignedIntegerRepresentation()) {
Result = Builder.CreateICmp(SICmpOpc, LHS, RHS, "cmp");
-} else {
- // Unsigned integers and pointers.
+} else { // Unsigned integers and pointers.
+ if (CGF.CGM.getCodeGenOpts().StrictVTablePointers) {
+// Based on comparisons of pointers to dynamic objects, the optimizer
+// can replace one pointer with another. This might result in
+// replacing pointer after barrier to pointer before barrier,
+// resulting in invalid devirtualization.
+if (auto *RD = LHSTy->getPointeeCXXRecordDecl())
+ if (!RD->isCompleteDefinition() || RD->isDynamicClass())
+LHS = Builder.CreateInvariantGroupBarrier(LHS);
+if (auto *RD = RHSTy->getPointeeCXXRecordDecl())
+ if (!RD->isCompleteDefinition() || RD->isDynamicClass())
+RHS = Builder.CreateInvariantGroupBarrier(RHS);
+ }
Result = Builder.CreateICmp(UICmpOpc, LHS, RHS, "cmp");
}
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons
Prazek marked an inline comment as done.
Prazek added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:3066-3067
+} else { // Unsigned integers and pointers.
+ if (CGF.CGM.getCodeGenOpts().StrictVTablePointers &&
+ CGF.CGM.getCodeGenOpts().OptimizationLevel > 0) {
+// Based on comparisons of pointers to dynamic objects, the optimizer
rsmith wrote:
> I think we need to do this regardless of optimization level -- if we LTO
> together a -O0 translation unit with a -O2 translation unit, we still need
> this protection for the comparisons in the -O0 TU.
>
> (IIRC we chose to make -fstrict-vtable-pointers an IR-level ABI break, but we
> shouldn't do the same thing for optimization level.)
sounds good
https://reviews.llvm.org/D32378
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D32378: Insert invariant.group.barrier for pointers comparisons
rsmith added inline comments.
Comment at: lib/CodeGen/CGExprScalar.cpp:3066-3067
+} else { // Unsigned integers and pointers.
+ if (CGF.CGM.getCodeGenOpts().StrictVTablePointers &&
+ CGF.CGM.getCodeGenOpts().OptimizationLevel > 0) {
+// Based on comparisons of pointers to dynamic objects, the optimizer
I think we need to do this regardless of optimization level -- if we LTO
together a -O0 translation unit with a -O2 translation unit, we still need this
protection for the comparisons in the -O0 TU.
(IIRC we chose to make -fstrict-vtable-pointers an IR-level ABI break, but we
shouldn't do the same thing for optimization level.)
https://reviews.llvm.org/D32378
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
