[PATCH] D13603: [Introduction] C++ Const object optimization in LLVM

Richard Smith via cfe-commits cfe-commits at lists.llvm.org
Wed Dec 2 17:04:20 PST 2015


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





More information about the cfe-commits mailing list