This revision was automatically updated to reflect the committed changes.
Closed by commit rC339438: [CodeGen] Merge equivalent block copy/helper
functions. (authored by ahatanak, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D50152?vs=160053&id=160110#toc
Repository:
rC
rjmccall accepted this revision.
rjmccall added a comment.
This revision is now accepted and ready to land.
Thanks, LGTM.
Repository:
rC Clang
https://reviews.llvm.org/D50152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.ll
ahatanak updated this revision to Diff 160053.
ahatanak marked an inline comment as done.
ahatanak added a comment.
Replace the two flags, EHOnly and DisposeCannotThrow, passed to
pushCaptureCleanup with a single flag ForCopyHelper.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1725
BlockFieldFlags Flags, bool EHOnly,
+ bool DisposeCannotThrow, VarDecl *Var,
CodeGenFunction &CGF) {
---
ahatanak added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1725
BlockFieldFlags Flags, bool EHOnly,
+ bool DisposeCannotThrow, VarDecl *Var,
CodeGenFunction &CGF) {
---
ahatanak updated this revision to Diff 160040.
ahatanak marked an inline comment as done.
ahatanak added a comment.
Define functions inline in the header file.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
include/clang/AST/ASTContext.h
lib/AST/ASTContext.cpp
lib/CodeGen/
rjmccall added inline comments.
Comment at: include/clang/AST/ASTContext.h:161
+Expr *getCopyExpr() const;
+bool canThrow() const;
+llvm::PointerIntPair ExprAndFlag;
These should all just be defined inline.
Comment at: lib/CodeGen/C
ahatanak added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1682
+ if (IsCopyHelper && Ctx.getBlockVarCopyInits(Var).CanThrow)
+Name += "c";
+}
rjmccall wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > I don't think you nee
ahatanak updated this revision to Diff 159926.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.
Address review comments.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
include/clang/AST/ASTContext.h
lib/AST/ASTContext.cpp
lib/CodeGen/CGBlocks.cpp
lib/
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1682
+ if (IsCopyHelper && Ctx.getBlockVarCopyInits(Var).CanThrow)
+Name += "c";
+}
ahatanak wrote:
> rjmccall wrote:
> > I don't think you need to add `d` to the na
ahatanak added inline comments.
Comment at: include/clang/AST/ASTContext.h:158
+Expr *CopyExpr;
+bool CanThrow;
+ };
rjmccall wrote:
> Using a PointerIntPair in the implementation of this is still a reasonable
> choice. Just make it a proper abstractio
ahatanak updated this revision to Diff 159850.
ahatanak marked 5 inline comments as done.
ahatanak added a comment.
Address review comments.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
include/clang/AST/ASTContext.h
lib/AST/ASTContext.cpp
lib/CodeGen/CGBlocks.cpp
lib/
rjmccall added inline comments.
Comment at: include/clang/AST/ASTContext.h:248
+ /// Mapping from __block VarDecls to their copy initialization expr. The
+ /// boolean flag indicates whether the expression can throw.
+ typedef llvm::DenseMap Maybe you should just make a type f
ahatanak updated this revision to Diff 159800.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.
Modify getBlockVarCopyInits and setBlockVarCopyInits to get and set both the
copy expression and the boolean flag that indicates whether the expression can
throw or not.
Reposito
rjmccall added inline comments.
Comment at: include/clang/AST/ASTContext.h:248
+ /// Mapping from __block VarDecls to their copy initialization expr. The
+ /// boolean flag indicates whether the expression can throw.
+ typedef llvm::DenseMaphttps://reviews.llvm.org/D50152
_
ahatanak updated this revision to Diff 159762.
ahatanak added a comment.
Change the value type of BlockVarCopyInits to PointerIntPair.
The boolean flag indicates whether the copy expression can throw.
Serialize/deserialize the copy expression and the boolean flag and add a
regression test to te
rjmccall added a comment.
In https://reviews.llvm.org/D50152#1191777, @ahatanak wrote:
> Since BlockVarCopyInits is a map with key `VarDecl *`, I think we want to add
> a flag to VarDecl (NonParmVarDeclBits) that indicates whether the copy
> expression can throw or not. Or is there a reason to
ahatanak added a comment.
Since BlockVarCopyInits is a map with key `VarDecl *`, I think we want to add a
flag to VarDecl (NonParmVarDeclBits) that indicates whether the copy expression
can throw or not. Or is there a reason to store the bit in `BlockDecl::Capture`
instead?
Repository:
rC C
ahatanak added a comment.
I thought it was okay to skip the work done by `ResolveExceptionSpec` in IRGen
as long as the exception specifications that are needed have already been
resolved in Sema. But calling Sema::canThrow in
Sema::CheckCompleteVariableDeclaration and storing the result in
Bl
rjmccall added a comment.
That is a change that Richard should definitely sign off on. Also, I'm not
sure this works — is it really okay to skip the work done by
`ResolveExceptionSpec` in IRGen? What does that mean, that we're just somewhat
more conservative than we would otherwise be? And w
ahatanak updated this revision to Diff 159553.
ahatanak added a comment.
Remove a stale comment and add an assertion to check the destructor's exception
specification has been resolved.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
include/clang/AST/ComputeExceptionSpec.h
ahatanak added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1643
+if (Ctx.getBlockVarCopyInits(VD))
+ return true;
+ return false;
rjmccall wrote:
> ahatanak wrote:
> > ahatanak wrote:
> > > rjmccall wrote:
> > > > Can you just ask Sema to chec
ahatanak updated this revision to Diff 159546.
ahatanak marked an inline comment as done.
ahatanak added a reviewer: rsmith.
ahatanak added a comment.
Herald added a subscriber: jfb.
Address review comments.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
include/clang/AST/Comp
rjmccall added a comment.
You might want to ask Richard on IRC if there are caveats when using that for
these purposes.
Repository:
rC Clang
https://reviews.llvm.org/D50152
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llv
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1643
+if (Ctx.getBlockVarCopyInits(VD))
+ return true;
+ return false;
ahatanak wrote:
> ahatanak wrote:
> > rjmccall wrote:
> > > Can you just ask Sema to check `canThrow` for the exp
ahatanak added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1643
+if (Ctx.getBlockVarCopyInits(VD))
+ return true;
+ return false;
ahatanak wrote:
> rjmccall wrote:
> > Can you just ask Sema to check `canThrow` for the expression and pass it
>
ahatanak added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1643
+if (Ctx.getBlockVarCopyInits(VD))
+ return true;
+ return false;
rjmccall wrote:
> Can you just ask Sema to check `canThrow` for the expression and pass it down?
Since this chang
ahatanak updated this revision to Diff 159388.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.
Address review comments.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGBlocks.h
lib/CodeGen/CGDecl.cpp
lib/CodeGen/
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1630
+if (const CXXDestructorDecl *DD = RD->getDestructor())
+ if (const auto FPT = DD->getType()->getAs())
+// Conservatively assume the destructor can throw if the exception
I
ahatanak added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1691
+
+Name += "_" + llvm::to_string(E.Capture->getOffset().getQuantity());
+ }
rjmccall wrote:
> I feel like this reads a little better if you write the quantity first. Also
> I think y
ahatanak updated this revision to Diff 159250.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.
Address review comments.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGBlocks.h
lib/CodeGen/CGDecl.cpp
lib/CodeGen/
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1691
+
+Name += "_" + llvm::to_string(E.Capture->getOffset().getQuantity());
+ }
I feel like this reads a little better if you write the quantity first. Also I
think you can drop the unde
ahatanak added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:492
+if (!RD->isExternallyVisible())
+ info.CapturesNonExternalType = true;
+
rjmccall wrote:
> You only need to do this if you're going to mangle the type name, i.e. if
> it's
ahatanak updated this revision to Diff 158892.
ahatanak marked 2 inline comments as done.
ahatanak added a comment.
Address review comments.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGBlocks.h
lib/CodeGen/CGNonTrivialStruct.cpp
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1638
+switch (E.Kind) {
+case BlockCaptureEntityKind::CXXRecord: {
+ Name += "c";
ahatanak wrote:
> rjmccall wrote:
> > I forget whether this case has already filtered out trivial
ahatanak added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1638
+switch (E.Kind) {
+case BlockCaptureEntityKind::CXXRecord: {
+ Name += "c";
rjmccall wrote:
> I forget whether this case has already filtered out trivial
> copy-constructors,
ahatanak updated this revision to Diff 158774.
ahatanak marked 3 inline comments as done.
ahatanak added a comment.
Address review comments.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGBlocks.h
lib/CodeGen/CGNonTrivialStruct.cpp
rjmccall added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1638
+switch (E.Kind) {
+case BlockCaptureEntityKind::CXXRecord: {
+ Name += "c";
I forget whether this case has already filtered out trivial copy-constructors,
but if not, you sho
ahatanak added inline comments.
Comment at: test/CodeGenCXX/block-byref-cxx-objc.cpp:36
+// CHECK: call void @_Block_object_dispose(
+
+int testB() {
ahatanak wrote:
> Should the second call to @_Block_object_dispose be an invoke if the
> destructor can throw? T
ahatanak updated this revision to Diff 158640.
ahatanak marked an inline comment as done.
ahatanak added a comment.
Use llvm::sort.
Repository:
rC Clang
https://reviews.llvm.org/D50152
Files:
lib/CodeGen/CGBlocks.cpp
lib/CodeGen/CGNonTrivialStruct.cpp
lib/CodeGen/CodeGenFunction.cpp
mgrang added inline comments.
Comment at: lib/CodeGen/CGBlocks.cpp:1593
+ // Sort the captures by offset.
+ std::sort(ManagedCaptures.begin(), ManagedCaptures.end());
}
Please use llvm::sort instead of std::sort. See
https://llvm.org/docs/CodingStandards.html
ahatanak added inline comments.
Comment at: test/CodeGenCXX/block-byref-cxx-objc.cpp:36
+// CHECK: call void @_Block_object_dispose(
+
+int testB() {
Should the second call to @_Block_object_dispose be an invoke if the destructor
can throw? The FIXME in CodeGenF
ahatanak created this revision.
ahatanak added reviewers: rjmccall, arphaman.
Herald added subscribers: dexonsmith, mgrang.
Currently, clang generates different copy or dispose helper functions for each
block literal on the stack if the block has the possibility of being copied to
the heap. This
43 matches
Mail list logo
