akhuang added a comment.

In D154007#4457561 <https://reviews.llvm.org/D154007#4457561>, @efriedma wrote:

> I'm not confident that isUsed() works the way you want it to in this context. 
>  In particular, if the code in question runs before we've translated the 
> whole translation unit, the isUsed() bit could change.  If you want that's 
> more obviously safe, you could just check if there are any captures.  (I'm 
> assuming the point of this is just to reduce the number of lambdas that go 
> through this codepath?)

Oh, thanks. Yeah, the point is we don't have to go down this path if we never 
have to emit the invoker.

In D154007#4457592 <https://reviews.llvm.org/D154007#4457592>, @rnk wrote:

> Can you please add a test case for the issue that caused the revert? I wanted 
> to dig into that to try to understand why the extra copy was being emitted. I 
> think you mentioned it has something to do with increasing the alignment.

oops, I added comments when I put up the patch but never submitted them.. 
there's a repro in crbug.com/1457256 but I'll also add a test case



================
Comment at: clang/lib/CodeGen/CGCall.cpp:5103
 
-        if (Addr.getAlignment() < Align &&
+        if (CallInfo.isDelegateCall()) {
+          NeedCopy = false;
----------------
I think the problem is that it tries to do a copy here because the alignment of 
the forwarding function arg is larger than the alignment of the object that's 
being passed. I'm not sure how alignments are computed or if there are any 
other requirements for alignment. Is it ok to just ignore the new alignment? Do 
we need to change the code that computes the argument alignment?

(crbug.com/1457256#comment2 has an example repro)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154007/new/

https://reviews.llvm.org/D154007

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to