rsmith added a subscriber: rsmith.
rsmith added a comment.
Your http://reviews.llvm.org/D14419 removes a lot of the code added here. Can
you update this patch to not add that code? I'd prefer not to review the
portion of this code that you're about to delete.
================
Comment at: lib/CodeGen/CGDecl.cpp:369
@@ -368,1 +368,3 @@
+ MarkWriteOnceWrittenRAII MWO(*this, &D, var);
+
----------------
The call to `AddInitializerToStaticVarDecl` above indirectly calls
`EmitCXXGlobalVarDeclInit`, which already emits an `llvm.invariant.start` call
when appropriate. Please extend the code in `EmitCXXGlobalVarDeclInit` to
handle these new cases (constant types with non-trivial destructors) rather
adding another codepath that creates an `llvm.invariant.start` here.
================
Comment at: lib/CodeGen/CGDecl.cpp:877-879
@@ +876,5 @@
+
+ // Only GlobalVariable's and AllocaInst's can be writeonce.
+ // Exit if the given address is none of these.
+ if (!GV && !AI) return;
+
----------------
I don't think this check is necessary; if we know some region of memory is
locally constant, I don't think it matters whether we can trace it back to an
alloca.
================
Comment at: lib/CodeGen/CGDecl.cpp:884
@@ +883,3 @@
+ // @llvm.invariant.end() intrinsic onto the cleanup stack.
+ if ((AI || !GV->isConstant()) && D->getType().isWriteOnce(CGF.getContext()))
+ Args = CGF.EmitInvariantStart(*D, AddrPtr);
----------------
Do we need to add `QualType::isWriteOnce` for this?
`CodeGenModule::isTypeConstant` seems to be doing mostly the right set of
checks. I suggest you add a new flag to it to indicate whether it should check
for trivial destructibility, and then use it here instead of using
`isWriteOnce`.
================
Comment at: lib/CodeGen/CGDecl.cpp:902
@@ -848,2 +901,3 @@
EmitAutoVarInit(emission);
+ MarkWriteOnceWrittenRAII MWO(*this, &D);
EmitAutoVarCleanups(emission);
----------------
The use of RAII here seems unnecessary. You should be able to emit the
invariant start and the invariant end cleanup at the same time, by moving this
after the call to `EmitAutoVarCleanups`.
================
Comment at: lib/CodeGen/CGDecl.cpp:952-958
@@ +951,9 @@
+ llvm::Value *Size;
+ if (llvm::Constant *CAddr = dyn_cast<llvm::Constant>(Addr)) {
+ Size = llvm::ConstantInt::getSigned(Int64Ty, Width);
+ Addr = llvm::ConstantExpr::getBitCast(CAddr, Int8PtrTy);
+ } else {
+ Size = llvm::ConstantInt::get(Int64Ty, Width);
+ Addr = Builder.CreateBitCast(Addr, Int8PtrTy);
+ }
+ llvm::CallInst *C = Builder.CreateCall(InvariantStart, {Size, Addr});
----------------
This conditional should not be necessary: `IRBuilder::CreateBitCast` should
choose between a bitcast constant and a bitcast instruction for you. Just use
the non-constant branch in all cases.
================
Comment at: lib/CodeGen/CGDecl.cpp:959
@@ +958,3 @@
+ }
+ llvm::CallInst *C = Builder.CreateCall(InvariantStart, {Size, Addr});
+ C->setDoesNotThrow();
----------------
Not all of our supported versions of MSVC accept this syntax. Create a separate
array of `llvm::Value *` and pass it in here.
================
Comment at: lib/CodeGen/CGDeclCXX.cpp:478
@@ -491,2 +477,3 @@
}
+ MarkWriteOnceWrittenRAII MWO(*this, D, Addr);
----------------
This should be unnecessary if you let `EmitCXXGlobalVarDeclInit` emit the
invariant start.
================
Comment at: test/CodeGenCXX/init-invariant.cpp:47-48
@@ -46,4 +46,4 @@
// CHECK: call void @_ZN1BC1Ev({{.*}}* nonnull @b)
-// CHECK-NOT: call {{.*}}@llvm.invariant.start(i64 4, i8* bitcast ({{.*}} @b
to i8*))
+// CHECK: call {{.*}}@llvm.invariant.start(i64 4, i8* bitcast ({{.*}} @b to
i8*))
----------------
Please also check that `@llvm.invariant.end` is called later in the same global
initialization function.
================
Comment at: test/CodeGenCXX/init-invariant.cpp:51
@@ -50,3 +50,3 @@
// CHECK: call void @_ZN1CC1Ev({{.*}}* nonnull @c)
-// CHECK-NOT: call {{.*}}@llvm.invariant.start(i64 4, i8* bitcast ({{.*}} @c
to i8*))
+// CHECK: call {{.*}}@llvm.invariant.start(i64 4, i8* bitcast ({{.*}} @c to
i8*))
----------------
Likewise for this case.
http://reviews.llvm.org/D13603
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
