[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
This revision was automatically updated to reflect the committed changes.
Closed by commit rL327939: [CodeGen] Ignore OpaqueValueExprs that are unique
references to their (authored by ahatanak, committed by ).
Herald added a subscriber: llvm-commits.
Changed prior to commit:
https://reviews.llvm.org/D39562?vs=137418=139067#toc
Repository:
rL LLVM
https://reviews.llvm.org/D39562
Files:
cfe/trunk/include/clang/AST/Expr.h
cfe/trunk/include/clang/AST/Stmt.h
cfe/trunk/lib/CodeGen/CGExpr.cpp
cfe/trunk/lib/CodeGen/CGExprAgg.cpp
cfe/trunk/lib/CodeGen/CGExprComplex.cpp
cfe/trunk/lib/CodeGen/CGExprScalar.cpp
cfe/trunk/lib/CodeGen/CodeGenFunction.h
cfe/trunk/lib/Sema/SemaPseudoObject.cpp
cfe/trunk/lib/Serialization/ASTReaderStmt.cpp
cfe/trunk/lib/Serialization/ASTWriterStmt.cpp
cfe/trunk/test/CodeGenCXX/ms-property.cpp
cfe/trunk/test/CodeGenObjC/objc-container-subscripting-1.m
cfe/trunk/test/CodeGenObjCXX/property-dot-copy-elision.mm
cfe/trunk/test/CodeGenObjCXX/property-objects.mm
Index: cfe/trunk/include/clang/AST/Stmt.h
===
--- cfe/trunk/include/clang/AST/Stmt.h
+++ cfe/trunk/include/clang/AST/Stmt.h
@@ -237,6 +237,16 @@
unsigned ResultIndex : 32 - 8 - NumExprBits;
};
+ class OpaqueValueExprBitfields {
+friend class OpaqueValueExpr;
+
+unsigned : NumExprBits;
+
+/// The OVE is a unique semantic reference to its source expressio if this
+/// bit is set to true.
+unsigned IsUnique : 1;
+ };
+
class ObjCIndirectCopyRestoreExprBitfields {
friend class ObjCIndirectCopyRestoreExpr;
@@ -294,6 +304,7 @@
CallExprBitfields CallExprBits;
ExprWithCleanupsBitfields ExprWithCleanupsBits;
PseudoObjectExprBitfields PseudoObjectExprBits;
+OpaqueValueExprBitfields OpaqueValueExprBits;
ObjCIndirectCopyRestoreExprBitfields ObjCIndirectCopyRestoreExprBits;
InitListExprBitfields InitListExprBits;
TypeTraitExprBitfields TypeTraitExprBits;
Index: cfe/trunk/include/clang/AST/Expr.h
===
--- cfe/trunk/include/clang/AST/Expr.h
+++ cfe/trunk/include/clang/AST/Expr.h
@@ -883,6 +883,7 @@
(SourceExpr && SourceExpr->isInstantiationDependent()),
false),
SourceExpr(SourceExpr), Loc(Loc) {
+setIsUnique(false);
}
/// Given an expression which invokes a copy constructor --- i.e. a
@@ -925,6 +926,14 @@
/// place.
Expr *getSourceExpr() const { return SourceExpr; }
+ void setIsUnique(bool V) {
+assert((!V || SourceExpr) &&
+ "unique OVEs are expected to have source expressions");
+OpaqueValueExprBits.IsUnique = V;
+ }
+
+ bool isUnique() const { return OpaqueValueExprBits.IsUnique; }
+
static bool classof(const Stmt *T) {
return T->getStmtClass() == OpaqueValueExprClass;
}
Index: cfe/trunk/test/CodeGenCXX/ms-property.cpp
===
--- cfe/trunk/test/CodeGenCXX/ms-property.cpp
+++ cfe/trunk/test/CodeGenCXX/ms-property.cpp
@@ -75,11 +75,11 @@
// CHECK: call void @"??$foo@H@@YAXHH@Z"(i32 %{{.+}}, i32 %{{.+}})
foo(argc, (int)argv[0][0]);
// CHECK: [[P2:%.+]] = load %class.St*, %class.St** %
- // CHECK: [[T_X:%.+]] = call i32 @"?get_x@Test1@@QEBAHXZ"(%class.Test1* %{{.+}})
// CHECK: [[P1:%.+]] = load %class.S*, %class.S** %
// CHECK: [[P1_X_22_33:%.+]] = call i32 @"?GetX@S@@QEAAHHH@Z"(%class.S* [[P1]], i32 22, i32 33)
// CHECK: [[CAST:%.+]] = sitofp i32 [[P1_X_22_33]] to double
// CHECK: [[ARGC:%.+]] = load i32, i32* %
+ // CHECK: [[T_X:%.+]] = call i32 @"?get_x@Test1@@QEBAHXZ"(%class.Test1* %{{.+}})
// CHECK: [[CAST2:%.+]] = trunc i32 [[T_X]] to i8
// CHECK: call void @"?PutY@?$St@M@@QEAAXDHN@Z"(%class.St* [[P2]], i8 [[CAST2]], i32 [[ARGC]], double [[CAST]])
p2->y[t.X][argc] = p1->x[22][33];
Index: cfe/trunk/test/CodeGenObjC/objc-container-subscripting-1.m
===
--- cfe/trunk/test/CodeGenObjC/objc-container-subscripting-1.m
+++ cfe/trunk/test/CodeGenObjC/objc-container-subscripting-1.m
@@ -25,8 +25,8 @@
// CHECK-NEXT: store i8* [[CALL]], i8** [[OLDOBJ:%.*]], align 8
val = (array[10] = oldObject);
-// CHECK: [[THREE:%.*]] = load {{%.*}} [[array:%.*]], align 8
-// CHECK-NEXT: [[FOUR:%.*]] = load i8*, i8** [[oldObject:%.*]], align 8
+// CHECK: [[FOUR:%.*]] = load i8*, i8** [[oldObject:%.*]], align 8
+// CHECK-NEXT: [[THREE:%.*]] = load {{%.*}} [[array:%.*]], align 8
// CHECK-NEXT: [[FIVE:%.*]] = load i8*, i8** @OBJC_SELECTOR_REFERENCES_.2
// CHECK-NEXT: [[SIX:%.*]] = bitcast {{%.*}} [[THREE]] to i8*
// CHECK-NEXT: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, i8*, i32)*)(i8* [[SIX]], i8* [[FIVE]], i8* [[FOUR]], i32 10)
@@ -45,9 +45,9 @@
val = (dictionary[key] = newObject);
-// CHECK: [[TWELVE:%.*]] = load {{%.*}} [[DICTIONARY]], align 8
+//
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
rjmccall accepted this revision. rjmccall added a comment. This revision is now accepted and ready to land. Alright, LGTM. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak added a comment. I also add a call to setIsUnique(false) in OpaqueValueExpr's constructor, which fixed the failing tests I was seeing. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak updated this revision to Diff 137418.
ahatanak marked an inline comment as done.
ahatanak added a comment.
Don't set the IsUnique bit of an OpaqueValueExpr that is used as the result
expression.
https://reviews.llvm.org/D39562
Files:
include/clang/AST/Expr.h
include/clang/AST/Stmt.h
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprAgg.cpp
lib/CodeGen/CGExprComplex.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaPseudoObject.cpp
lib/Serialization/ASTReaderStmt.cpp
lib/Serialization/ASTWriterStmt.cpp
test/CodeGenCXX/ms-property.cpp
test/CodeGenObjC/objc-container-subscripting-1.m
test/CodeGenObjCXX/property-dot-copy-elision.mm
test/CodeGenObjCXX/property-objects.mm
Index: test/CodeGenObjCXX/property-objects.mm
===
--- test/CodeGenObjCXX/property-objects.mm
+++ test/CodeGenObjCXX/property-objects.mm
@@ -119,11 +119,11 @@
// CHECK:define void @_Z6testB0P1B([[B:%.*]]*
// CHECK: [[BVAR:%.*]] = alloca [[B]]*, align 8
// CHECK: [[TEMP:%.*]] = alloca [[B0:%.*]], align 8
-// CHECK: load [[B]]*, [[B]]** [[BVAR]]
-// CHECK-NEXT: [[X:%.*]] = getelementptr inbounds [[B0]], [[B0]]* [[TEMP]], i32 0, i32 0
+// CHECK: [[X:%.*]] = getelementptr inbounds [[B0]], [[B0]]* [[TEMP]], i32 0, i32 0
// CHECK-NEXT: [[T0:%.*]] = call i32 @_Z9b_makeIntv()
// CHECK-NEXT: [[T1:%.*]] = sext i32 [[T0]] to i64
// CHECK-NEXT: store i64 [[T1]], i64* [[X]], align 8
+// CHECK: load [[B]]*, [[B]]** [[BVAR]]
// CHECK-NOT: call
// CHECK: call void @llvm.memcpy
// CHECK-NOT: call
@@ -161,12 +161,12 @@
// CHECK:define void @_Z6testB2P1B([[B]]*
// CHECK: [[BVAR:%.*]] = alloca [[B]]*, align 8
-// CHECK: load [[B]]*, [[B]]** [[BVAR]]
-// CHECK-NOT: call
+// CHECK: call void @llvm.dbg.declare(
// CHECK: call void @_ZN2B3C1Ev(
// CHECK-NEXT: [[T0:%.*]] = call i64 @_ZN2B3cv2B1Ev(
// CHECK-NOT: call
// CHECK: store i64 [[T0]],
+// CHECK: load [[B]]*, [[B]]** [[BVAR]]
// CHECK-NOT: call
// CHECK: call void @llvm.memcpy
// CHECK-NOT: call
Index: test/CodeGenObjCXX/property-dot-copy-elision.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/property-dot-copy-elision.mm
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -std=c++1z -fobjc-arc -o - %s | FileCheck %s
+
+struct S0 {
+ id f;
+};
+
+struct S1 {
+ S1();
+ S1(S0);
+ id f;
+};
+
+@interface C
+@property S1 f;
+@end
+@implementation C
+@end
+
+// CHECK-LABEL: define void @_Z5test0P1C(
+// CHECK: %{{.*}} = alloca %
+// CHECK: %[[AGG_TMP:.*]] = alloca %[[STRUCT_S1:.*]], align
+// CHECK: %[[AGG_TMP_1:.*]] = alloca %[[STRUCT_S0:.*]], align
+// CHECK: call void @_ZN2S0C1Ev(%[[STRUCT_S0]]* %[[AGG_TMP_1]])
+// CHECK: call void @_ZN2S1C1E2S0(%[[STRUCT_S1]]* %[[AGG_TMP]], %[[STRUCT_S0]]* %[[AGG_TMP_1]])
+// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %[[STRUCT_S1]]*)*)(i8* %{{.*}}, i8* %{{.*}}, %[[STRUCT_S1]]* %[[AGG_TMP]])
+
+void test0(C *c) {
+ c.f = S0();
+}
+
+// CHECK: define void @_Z5test1P1C(
+// CHECK: %{{.*}} = alloca %
+// CHECK: %[[TEMP_LVALUE:.*]] = alloca %[[STRUCT_S1:.*]], align
+// CHECK: call void @_ZN2S1C1Ev(%[[STRUCT_S1]]* %[[TEMP_LVALUE]])
+// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %[[STRUCT_S1]]*)*)(i8* %{{.*}}, i8* %{{.*}}, %[[STRUCT_S1]]* %[[TEMP_LVALUE]])
+
+void test1(C *c) {
+ c.f = S1();
+}
Index: test/CodeGenObjC/objc-container-subscripting-1.m
===
--- test/CodeGenObjC/objc-container-subscripting-1.m
+++ test/CodeGenObjC/objc-container-subscripting-1.m
@@ -25,8 +25,8 @@
// CHECK-NEXT: store i8* [[CALL]], i8** [[OLDOBJ:%.*]], align 8
val = (array[10] = oldObject);
-// CHECK: [[THREE:%.*]] = load {{%.*}} [[array:%.*]], align 8
-// CHECK-NEXT: [[FOUR:%.*]] = load i8*, i8** [[oldObject:%.*]], align 8
+// CHECK: [[FOUR:%.*]] = load i8*, i8** [[oldObject:%.*]], align 8
+// CHECK-NEXT: [[THREE:%.*]] = load {{%.*}} [[array:%.*]], align 8
// CHECK-NEXT: [[FIVE:%.*]] = load i8*, i8** @OBJC_SELECTOR_REFERENCES_.2
// CHECK-NEXT: [[SIX:%.*]] = bitcast {{%.*}} [[THREE]] to i8*
// CHECK-NEXT: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, i8*, i32)*)(i8* [[SIX]], i8* [[FIVE]], i8* [[FOUR]], i32 10)
@@ -45,9 +45,9 @@
val = (dictionary[key] = newObject);
-// CHECK: [[TWELVE:%.*]] = load {{%.*}} [[DICTIONARY]], align 8
+// CHECK: [[FOURTEEN:%.*]] = load i8*, i8** [[NEWOBJECT:%.*]], align 8
+// CHECK-NEXT: [[TWELVE:%.*]] = load {{%.*}} [[DICTIONARY]], align 8
// CHECK-NEXT: [[THIRTEEN:%.*]] = load i8*, i8** [[KEY]], align 8
-// CHECK-NEXT: [[FOURTEEN:%.*]] = load i8*, i8** [[NEWOBJECT:%.*]], align 8
// CHECK-NEXT: [[SIXTEEN:%.*]] = load i8*, i8** @OBJC_SELECTOR_REFERENCES_.6
// CHECK-NEXT:
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak added inline comments. Comment at: lib/CodeGen/CGExpr.cpp:4815 +} + } + rjmccall wrote: > Oh! So it's an interesting point that the RHS might be used as the result > expression, which means its use might not really be unique anymore. It > happens to work in some sense for non-trivial C++ class types (when they're > passed indirectly, as under the Itanium ABI) because the temporary outlives > the call; on the other hand, the call is allowed to mutate its argument, and > it's not clear that it's okay to have those mutations be reflected in code > that's using the result of the assignment. Similarly, managed types (like > non-trivial C structs, or ARC pointers) might be consumed by the call; if > we're going to pass them, perhaps we need to copy the value before doing so. > > What do you think? > > I think the technical implication is that what you're doing here shouldn't be > necessary; the OVE arguably should not be unique if its value is used in > multiple places, and that includes being used as a result. I made changes so that OVEs used as the result expression are not marked as unique. I wasn't sure whether treating such OVEs as unique would actually cause problems, but it's probably better to be on the safe side. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak added a comment. Sorry, this patch is not ready yet. There are several regression tests that are failing because of the assert in setIsUnique. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
rjmccall added inline comments. Comment at: include/clang/AST/Expr.h:875 + /// is set to true. + bool IsUnique = false; + rjmccall wrote: > Humor me and pack this in the bitfields in Stmt, please. :) Thanks! Comment at: lib/CodeGen/CGExpr.cpp:4815 +} + } + Oh! So it's an interesting point that the RHS might be used as the result expression, which means its use might not really be unique anymore. It happens to work in some sense for non-trivial C++ class types (when they're passed indirectly, as under the Itanium ABI) because the temporary outlives the call; on the other hand, the call is allowed to mutate its argument, and it's not clear that it's okay to have those mutations be reflected in code that's using the result of the assignment. Similarly, managed types (like non-trivial C structs, or ARC pointers) might be consumed by the call; if we're going to pass them, perhaps we need to copy the value before doing so. What do you think? I think the technical implication is that what you're doing here shouldn't be necessary; the OVE arguably should not be unique if its value is used in multiple places, and that includes being used as a result. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak added inline comments. Comment at: lib/Sema/SemaPseudoObject.cpp:432 + if (capturedRHS->getType()->getAsCXXRecordDecl() && capturedRHS->isRValue()) +capturedRHS->setIsUnique(); + rjmccall wrote: > I think you can unconditionally set this here, actually. You just need to > teach the other two exhaustive emitters in IRGen (scalar and complex) to look > through unique OVEs. Plenty of other things in IRGen could benefit from > being able to peephole through unique OVEs. > > Also, you can set it on the OVE for the base expression if this is a simple > assignment or load. All the builders created in checkPseudoObjectRValue and checkPseudoObjectAssignment (only when the assignment is simple) set the OVE's IsUnique bit to true. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak updated this revision to Diff 137250.
ahatanak marked 3 inline comments as done.
ahatanak added a comment.
Address review comments.
https://reviews.llvm.org/D39562
Files:
include/clang/AST/Expr.h
include/clang/AST/Stmt.h
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprAgg.cpp
lib/CodeGen/CGExprComplex.cpp
lib/CodeGen/CGExprScalar.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaPseudoObject.cpp
lib/Serialization/ASTReaderStmt.cpp
lib/Serialization/ASTWriterStmt.cpp
test/CodeGenCXX/ms-property.cpp
test/CodeGenObjC/objc-container-subscripting-1.m
test/CodeGenObjCXX/property-dot-copy-elision.mm
test/CodeGenObjCXX/property-objects.mm
Index: test/CodeGenObjCXX/property-objects.mm
===
--- test/CodeGenObjCXX/property-objects.mm
+++ test/CodeGenObjCXX/property-objects.mm
@@ -72,7 +72,7 @@
// rdar://8379892
// CHECK-LABEL: define void @_Z1fP1A
-// CHECK: call void @_ZN1XC1Ev(%struct.X* [[LVTEMP:%[a-zA-Z0-9\.]+]])
+// CHECK: call void @_ZN1XC1Ev(%struct.X* [[LVTEMP:%.+]])
// CHECK: call void @_ZN1XC1ERKS_(%struct.X* [[AGGTMP:%[a-zA-Z0-9\.]+]], %struct.X* dereferenceable({{[0-9]+}}) [[LVTEMP]])
// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %struct.X*)*)({{.*}} %struct.X* [[AGGTMP]])
struct X {
@@ -118,15 +118,14 @@
}
// CHECK:define void @_Z6testB0P1B([[B:%.*]]*
// CHECK: [[BVAR:%.*]] = alloca [[B]]*, align 8
-// CHECK: [[TEMP:%.*]] = alloca [[B0:%.*]], align 8
+// CHECK: alloca [[B0:%.*]], align 8
+// CHECK: [[TEMP:%.*]] = alloca [[B0]], align 8
// CHECK: load [[B]]*, [[B]]** [[BVAR]]
// CHECK-NEXT: [[X:%.*]] = getelementptr inbounds [[B0]], [[B0]]* [[TEMP]], i32 0, i32 0
// CHECK-NEXT: [[T0:%.*]] = call i32 @_Z9b_makeIntv()
// CHECK-NEXT: [[T1:%.*]] = sext i32 [[T0]] to i64
// CHECK-NEXT: store i64 [[T1]], i64* [[X]], align 8
// CHECK-NOT: call
-// CHECK: call void @llvm.memcpy
-// CHECK-NOT: call
// CHECK: call void bitcast {{.*}} @objc_msgSend
// CHECK-NOT: call
// CHECK: ret void
@@ -168,8 +167,6 @@
// CHECK-NOT: call
// CHECK: store i64 [[T0]],
// CHECK-NOT: call
-// CHECK: call void @llvm.memcpy
-// CHECK-NOT: call
// CHECK: call void bitcast {{.*}} @objc_msgSend
// CHECK-NOT: call
// CHECK: ret void
Index: test/CodeGenObjCXX/property-dot-copy-elision.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/property-dot-copy-elision.mm
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -std=c++1z -fobjc-arc -o - %s | FileCheck %s
+
+struct S0 {
+ id f;
+};
+
+struct S1 {
+ S1();
+ S1(S0);
+ id f;
+};
+
+@interface C
+@property S1 f;
+@end
+@implementation C
+@end
+
+// CHECK-LABEL: define void @_Z5test0P1C(
+// CHECK: %{{.*}} = alloca %
+// CHECK: %[[AGG_TMP:.*]] = alloca %[[STRUCT_S1:.*]], align
+// CHECK: %[[AGG_TMP_1:.*]] = alloca %[[STRUCT_S0:.*]], align
+// CHECK: call void @_ZN2S0C1Ev(%[[STRUCT_S0]]* %[[AGG_TMP_1]])
+// CHECK: call void @_ZN2S1C1E2S0(%[[STRUCT_S1]]* %[[AGG_TMP]], %[[STRUCT_S0]]* %[[AGG_TMP_1]])
+// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %[[STRUCT_S1]]*)*)(i8* %{{.*}}, i8* %{{.*}}, %[[STRUCT_S1]]* %[[AGG_TMP]])
+
+void test0(C *c) {
+ c.f = S0();
+}
+
+// CHECK: define void @_Z5test1P1C(
+// CHECK: %{{.*}} = alloca %
+// CHECK: %[[TEMP_LVALUE:.*]] = alloca %[[STRUCT_S1:.*]], align
+// CHECK: call void @_ZN2S1C1Ev(%[[STRUCT_S1]]* %[[TEMP_LVALUE]])
+// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %[[STRUCT_S1]]*)*)(i8* %{{.*}}, i8* %{{.*}}, %[[STRUCT_S1]]* %[[TEMP_LVALUE]])
+
+void test1(C *c) {
+ c.f = S1();
+}
Index: test/CodeGenObjC/objc-container-subscripting-1.m
===
--- test/CodeGenObjC/objc-container-subscripting-1.m
+++ test/CodeGenObjC/objc-container-subscripting-1.m
@@ -46,8 +46,8 @@
val = (dictionary[key] = newObject);
// CHECK: [[TWELVE:%.*]] = load {{%.*}} [[DICTIONARY]], align 8
-// CHECK-NEXT: [[THIRTEEN:%.*]] = load i8*, i8** [[KEY]], align 8
// CHECK-NEXT: [[FOURTEEN:%.*]] = load i8*, i8** [[NEWOBJECT:%.*]], align 8
+// CHECK-NEXT: [[THIRTEEN:%.*]] = load i8*, i8** [[KEY]], align 8
// CHECK-NEXT: [[SIXTEEN:%.*]] = load i8*, i8** @OBJC_SELECTOR_REFERENCES_.6
// CHECK-NEXT: [[SEVENTEEN:%.*]] = bitcast {{%.*}} [[TWELVE]] to i8*
// CHECK-NEXT: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, i8*, i8*)*)(i8* [[SEVENTEEN]], i8* [[SIXTEEN]], i8* [[FOURTEEN]], i8* [[THIRTEEN]])
Index: test/CodeGenCXX/ms-property.cpp
===
--- test/CodeGenCXX/ms-property.cpp
+++ test/CodeGenCXX/ms-property.cpp
@@ -75,11 +75,11 @@
// CHECK: call void @"\01??$foo@H@@YAXHH@Z"(i32 %{{.+}}, i32 %{{.+}})
foo(argc, (int)argv[0][0]);
// CHECK:
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
rjmccall added a comment. Oh, and you need to serialize this bit. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
rjmccall added inline comments.
Comment at: include/clang/AST/Expr.h:875
+ /// is set to true.
+ bool IsUnique = false;
+
Humor me and pack this in the bitfields in Stmt, please. :)
Comment at: include/clang/AST/Expr.h:932
+ void setIsUnique() { IsUnique = true; }
+ bool isUnique() const { return IsUnique; }
Can we assert that there's a source expression?
Comment at: lib/Sema/SemaPseudoObject.cpp:432
+ if (capturedRHS->getType()->getAsCXXRecordDecl() && capturedRHS->isRValue())
+capturedRHS->setIsUnique();
+
I think you can unconditionally set this here, actually. You just need to
teach the other two exhaustive emitters in IRGen (scalar and complex) to look
through unique OVEs. Plenty of other things in IRGen could benefit from being
able to peephole through unique OVEs.
Also, you can set it on the OVE for the base expression if this is a simple
assignment or load.
https://reviews.llvm.org/D39562
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak updated this revision to Diff 136895.
ahatanak added a comment.
Add a flag to OpaqeuValueExpr that indicates whether it is a unique reference
to its subexpression. If the flag is set, IRGen will avoid binding the OVE and
copying the result to the destination and instead just visit the OVE's
sub-expression.
The flag is set only when the PseudoObjectExpr is an assignment and the RHS is
an rvalue of a C++ class type, which is sufficient to fix the assertion failure
this patch is trying to fix.
https://reviews.llvm.org/D39562
Files:
include/clang/AST/Expr.h
lib/CodeGen/CGExpr.cpp
lib/CodeGen/CGExprAgg.cpp
lib/CodeGen/CodeGenFunction.h
lib/Sema/SemaPseudoObject.cpp
test/CodeGenObjCXX/property-dot-copy-elision.mm
test/CodeGenObjCXX/property-objects.mm
Index: test/CodeGenObjCXX/property-objects.mm
===
--- test/CodeGenObjCXX/property-objects.mm
+++ test/CodeGenObjCXX/property-objects.mm
@@ -72,7 +72,7 @@
// rdar://8379892
// CHECK-LABEL: define void @_Z1fP1A
-// CHECK: call void @_ZN1XC1Ev(%struct.X* [[LVTEMP:%[a-zA-Z0-9\.]+]])
+// CHECK: call void @_ZN1XC1Ev(%struct.X* [[LVTEMP:%.+]])
// CHECK: call void @_ZN1XC1ERKS_(%struct.X* [[AGGTMP:%[a-zA-Z0-9\.]+]], %struct.X* dereferenceable({{[0-9]+}}) [[LVTEMP]])
// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %struct.X*)*)({{.*}} %struct.X* [[AGGTMP]])
struct X {
Index: test/CodeGenObjCXX/property-dot-copy-elision.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/property-dot-copy-elision.mm
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -std=c++1z -fobjc-arc -o - %s | FileCheck %s
+
+struct S0 {
+ id f;
+};
+
+struct S1 {
+ S1();
+ S1(S0);
+ id f;
+};
+
+@interface C
+@property S1 f;
+@end
+@implementation C
+@end
+
+// CHECK-LABEL: define void @_Z5test0P1C(
+// CHECK: %{{.*}} = alloca %
+// CHECK: %[[AGG_TMP:.*]] = alloca %[[STRUCT_S1:.*]], align
+// CHECK: %[[AGG_TMP_1:.*]] = alloca %[[STRUCT_S0:.*]], align
+// CHECK: call void @_ZN2S0C1Ev(%[[STRUCT_S0]]* %[[AGG_TMP_1]])
+// CHECK: call void @_ZN2S1C1E2S0(%[[STRUCT_S1]]* %[[AGG_TMP]], %[[STRUCT_S0]]* %[[AGG_TMP_1]])
+// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %[[STRUCT_S1]]*)*)(i8* %{{.*}}, i8* %{{.*}}, %[[STRUCT_S1]]* %[[AGG_TMP]])
+
+void test0(C *c) {
+ c.f = S0();
+}
+
+// CHECK: define void @_Z5test1P1C(
+// CHECK: %{{.*}} = alloca %
+// CHECK: %[[TEMP_LVALUE:.*]] = alloca %[[STRUCT_S1:.*]], align
+// CHECK: call void @_ZN2S1C1Ev(%[[STRUCT_S1]]* %[[TEMP_LVALUE]])
+// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %[[STRUCT_S1]]*)*)(i8* %{{.*}}, i8* %{{.*}}, %[[STRUCT_S1]]* %[[TEMP_LVALUE]])
+
+void test1(C *c) {
+ c.f = S1();
+}
Index: lib/Sema/SemaPseudoObject.cpp
===
--- lib/Sema/SemaPseudoObject.cpp
+++ lib/Sema/SemaPseudoObject.cpp
@@ -428,6 +428,9 @@
Expr *syntacticLHS = rebuildAndCaptureObject(LHS);
OpaqueValueExpr *capturedRHS = capture(RHS);
+ if (capturedRHS->getType()->getAsCXXRecordDecl() && capturedRHS->isRValue())
+capturedRHS->setIsUnique();
+
// In some very specific cases, semantic analysis of the RHS as an
// expression may require it to be rewritten. In these cases, we
// cannot safely keep the OVE around. Fortunately, we don't really
Index: lib/CodeGen/CodeGenFunction.h
===
--- lib/CodeGen/CodeGenFunction.h
+++ lib/CodeGen/CodeGenFunction.h
@@ -2124,14 +2124,7 @@
/// getOpaqueLValueMapping - Given an opaque value expression (which
/// must be mapped to an l-value), return its mapping.
- const LValue (const OpaqueValueExpr *e) {
-assert(OpaqueValueMapping::shouldBindAsLValue(e));
-
-llvm::DenseMap::iterator
- it = OpaqueLValues.find(e);
-assert(it != OpaqueLValues.end() && "no mapping for opaque value!");
-return it->second;
- }
+ LValue getOpaqueLValueMapping(const OpaqueValueExpr *e);
/// getOpaqueRValueMapping - Given an opaque value expression (which
/// must be mapped to an r-value), return its mapping.
Index: lib/CodeGen/CGExprAgg.cpp
===
--- lib/CodeGen/CGExprAgg.cpp
+++ lib/CodeGen/CGExprAgg.cpp
@@ -606,7 +606,11 @@
}
void AggExprEmitter::VisitOpaqueValueExpr(OpaqueValueExpr *e) {
- EmitFinalDestCopy(e->getType(), CGF.getOpaqueLValueMapping(e));
+ // Avoid copying if OVE is a unique reference to its source expression.
+ if (e->isUnique())
+Visit(e->getSourceExpr());
+ else
+EmitFinalDestCopy(e->getType(), CGF.getOpaqueLValueMapping(e));
}
void
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -4170,6 +4170,18
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak added a comment. I'm planning to work on a new patch this week. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
smeenai added a comment. Any updates here? We were hitting a similar error internally, which this patch fixed. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
rjmccall added a comment. Hmm. The OVE-ing of the RHS of a property assignment is just there to make the original source structure clearer. Maybe the right solution here is to set a flag in the OVE that says that it's a unique semantic reference to its source expression, and then change IRGen to just recurse through OVEs with that flag set (and not pre-bind it). You should make sure that we don't get this assertion with mandatory copy-elision and ?:, which does actually use its LHS multiple times semantically (and which cannot safely perform mandatory copy-elision on it). https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak added a comment. This is tracked by rdar://problem/34363596. https://reviews.llvm.org/D39562 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D39562: [CodeGen][ObjC] Fix an assertion failure caused by copy elision
ahatanak created this revision.
Herald added subscribers: kristof.beyls, aemerson.
The assertion failure occurs when a setter is called using the dot syntax to
set a property of a class type that doesn't have trivial copy/move constructors
or assignment operators. It happens only when clang is compiling for c++1z and
copy elision allows avoiding copying a temporary to the argument passed to the
setter method in Sema.
For example:
struct S2 {
id f1;
};
@interface C
@property S2 f;
@end
@implementation C
@end
void test(C *c) {
c.f = S2();
}
When IRGen visits the ObjCMessageExpr for the call to the setter, it tries to
emit a copy of an S2 object that has been constructed (this happens in
AggExprEmitter::VisitOpaqueValueExpr) and the assertion in
CodeGenFunction::EmitAggregateCopy fails because S2 doesn't have the trivial
special functions needed to do the copy.
This is the AST of the ObjCMessageExpr:
`-ObjCMessageExpr 0x7fe03c06b730 'void' selector=setF:
| |-OpaqueValueExpr 0x7fe03c06b698 'C *'
| | `-ImplicitCastExpr 0x7fe03c06b5e0 'C *'
| | `-DeclRefExpr 0x7fe03c06b5b8 'C *__strong' lvalue
ParmVar 0x7fe03c06b408 'c' 'C *__strong'
| `-OpaqueValueExpr 0x7fe03c06b6e8 'struct S2'
| `-CXXBindTemporaryExpr 0x7fe03c06b678 'struct S2'
(CXXTemporary 0x7fe03c06b670)
| `-CXXTemporaryObjectExpr 0x7fe03c06b638 'struct
S2' 'void (void)'
To avoid the crash, I modified CodeGenFunction::EmitAnyExprToTemp to return the
value for the OpaqueValueExpr (which must have already been evaluated when it's
visited, I think) so that it doesn't have to call EmitAggregateCopy to make a
copy.
I'm actually unsure whether we should fix Sema and change the AST
representation or fix IRGen as I did in this patch. I'm open to suggestions if
anyone has a better idea to fix the crash.
https://reviews.llvm.org/D39562
Files:
lib/CodeGen/CGExpr.cpp
test/CodeGenObjCXX/property-dot-copy-elision.mm
test/CodeGenObjCXX/property-objects.mm
Index: test/CodeGenObjCXX/property-objects.mm
===
--- test/CodeGenObjCXX/property-objects.mm
+++ test/CodeGenObjCXX/property-objects.mm
@@ -125,8 +125,6 @@
// CHECK-NEXT: [[T1:%.*]] = sext i32 [[T0]] to i64
// CHECK-NEXT: store i64 [[T1]], i64* [[X]], align 8
// CHECK-NOT: call
-// CHECK: call void @llvm.memcpy
-// CHECK-NOT: call
// CHECK: call void bitcast {{.*}} @objc_msgSend
// CHECK-NOT: call
// CHECK: ret void
@@ -147,8 +145,6 @@
// CHECK-NOT: call
// CHECK: store i64 [[T0]],
// CHECK-NOT: call
-// CHECK: call void @llvm.memcpy
-// CHECK-NOT: call
// CHECK: call void bitcast {{.*}} @objc_msgSend
// CHECK-NOT: call
// CHECK: ret void
@@ -168,8 +164,6 @@
// CHECK-NOT: call
// CHECK: store i64 [[T0]],
// CHECK-NOT: call
-// CHECK: call void @llvm.memcpy
-// CHECK-NOT: call
// CHECK: call void bitcast {{.*}} @objc_msgSend
// CHECK-NOT: call
// CHECK: ret void
@@ -216,8 +210,6 @@
// CHECK-NOT: call
// CHECK: store i32 [[T0]],
// CHECK-NOT: call
-// CHECK: call void @llvm.memcpy
-// CHECK-NOT: call
// CHECK: call void bitcast {{.*}} @objc_msgSend
// CHECK-NOT: call
// CHECK: ret void
Index: test/CodeGenObjCXX/property-dot-copy-elision.mm
===
--- /dev/null
+++ test/CodeGenObjCXX/property-dot-copy-elision.mm
@@ -0,0 +1,39 @@
+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -std=c++1z -fobjc-arc -o - %s | FileCheck %s
+
+struct S0 {
+ id f;
+};
+
+struct S1 {
+ S1();
+ S1(S0);
+ id f;
+};
+
+@interface C
+@property S1 f;
+@end
+@implementation C
+@end
+
+// CHECK-LABEL: define void @_Z5test0P1C(
+// CHECK: %{{.*}} = alloca %
+// CHECK: %[[TEMP_LVALUE:.*]] = alloca %[[STRUCT_S0:.*]], align
+// CHECK: %[[AGG_TMP:.*]] = alloca %[[STRUCT_S1:.*]], align
+// CHECK: call void @_ZN2S0C1Ev(%[[STRUCT_S0]]* %[[TEMP_LVALUE]])
+// CHECK: call void @_ZN2S1C1E2S0(%[[STRUCT_S1]]* %[[AGG_TMP]], %[[STRUCT_S0]]* %[[TEMP_LVALUE]])
+// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %[[STRUCT_S1]]*)*)(i8* %{{.*}}, i8* %{{.*}}, %[[STRUCT_S1]]* %[[AGG_TMP]])
+
+void test0(C *c) {
+ c.f = S0();
+}
+
+// CHECK: define void @_Z5test1P1C(
+// CHECK: %{{.*}} = alloca %
+// CHECK: %[[TEMP_LVALUE:.*]] = alloca %[[STRUCT_S1:.*]], align
+// CHECK: call void @_ZN2S1C1Ev(%[[STRUCT_S1]]* %[[TEMP_LVALUE]])
+// CHECK: call void bitcast (i8* (i8*, i8*, ...)* @objc_msgSend to void (i8*, i8*, %[[STRUCT_S1]]*)*)(i8* %{{.*}}, i8* %{{.*}}, %[[STRUCT_S1]]* %[[TEMP_LVALUE]])
+
+void test1(C *c) {
+ c.f = S1();
+}
Index: lib/CodeGen/CGExpr.cpp
===
--- lib/CodeGen/CGExpr.cpp
+++ lib/CodeGen/CGExpr.cpp
@@ -192,8 +192,12 @@
RValue
