[PATCH] D154007: Reland "Try to implement lambdas with inalloca parameters by forwarding without use of inallocas."

Wed Jun 28 16:00:16 PDT 2023

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