Thank you. Given that proceeding with the wrong value will result in either an undefined reference or a reference to what might well be the wrong function at link time, I think making this an error (as you've done) is strictly better.

Can you clarify "which will usually result in a linker error"? E.g. an example of link.exe rejecting the object file or the wrong function being called. The reason I ask is that if we can be sure at compile time that the resulting code will not work or do the wrong thing, I think giving an error is appropriate. But if that isn't the case, we would be rejecting code that cl.exe accepts and we might want to make it a Warning instead.

I think r320996 implements this.

Reverted in r360955.

I also think we should revert this change while we think about the approach. I've been reluctant to do this because I was impressed by how many bytes of output it saves, and I didn't want to lose that just because it doesn't play nice with ThinLTO. However, after some more data gathering and talking to people, I think that:

I put this up for preservation purposes, but I'm not convinced that we should actually go this route, so abandoning.

added symbols and check they are listed

Thanks for helping me think about this and alternative approaches. I'll be withdrawing this for now to get it out of the review queue. If I end up implementing a new approach, I'll put it up as a new diff (with a link to this one for the comments).

• don't hoist allocas if they are not in the same BB as the function that uses them

I think Eli raises a good point, and I'm inclined to just say "if the alloca (before hoisting) is not in the same BB as the call, don't hoist it". That avoids changing the stack behavior of the code. It also means we miss out on an optimization, but it's an optimization that we're not performing today anyway.

• updated with more sensible test case
• improvements suggested by @rnk

Correcting earlier upload to preserve original inalloca.ll tests and
add new tests in a separate file.

Remove inallocas in globalopt and simplify argpromotion by just
bailing on inallocas.

Yeah, I kind of wanted to do the least intrusive thing that would fix the problem I observed and then think about the larger picture. But given that we're thinking about removing the inalloca attribute, the way I would like to do that is remove it in globalopt and then we can perform further optimizations (in globalopt and here) predicated on no inalloca being present. That takes care of half the FIXME for inalloca in globalopt, so after that it becomes really tempting to do follow-up with the other half and hoist the alloca to the entry block.

Updated in response to @efriedma's comments. We now don't run
argpromotion on functions with inallocas. I've deleted some code
(mostly comments) that refers to them, and modified the test
which previously checked that we do perform the optimization in some
cases to instead check that we don't perform the optimization in
the case I found to be unsafe.

Eli, I saw after I wrote this that you suggested a slightly different approach on the bug (skip argpromote for functions with inallocas). If you'd rather have me do that instead of the change here, I'd be happy to. I just figured I'd put up the one I had already written for consideration.

Does this look good now?

without second constructor

Updated as suggested by @rsmith (thanks!)

Use [=] instead of explicitly passing StringRefs to ReportBufferError.

(Following the same archive(member) format also used by toString() in InputFiles.cpp)

Before:

lld-link: error: could not get the buffer for the member defining symbol spvDiagnosticDestroy: No such file or directory

LGTM

See earlier comment.

lgtm, thanks!

LGTM, with some suggestions in the comments.

Thank you!

Re-upload only the test change.

Simpler test. This crashes without D57073 and passes with D57073.

pcc, does D57192 implement your suggestion?

Just saw your comment, pcc. I'll look into that.

add missing .1

replaced .c test with an .ll test

@fedor.sergeev: You're right, thanks for pointing that out. Sent D56336 to fix that.

Thanks! For the case where you don't want the files to expire, can you instead set the expiration age to 0? That should stop the pruner from removing them.

LGTM

I was thinking that as well, and it did fix the size regression: from 151,548,928 to 141,140,992 bytes, where the non-LTO version is 140,072,960 bytes.

Since r338767, "foo,p" and "bar,p" show up as "foo,px" and "bar,px", indicating that they are used in regular objects. However, there are no regular (non-LTO) objects in the test. Only main should have the 'x'.

added test

Building Chromium with ThinLTO without this patch causes test_chrome_with_chromedriver.py to fail. With the patch, it runs successfully. (https://crbug.com/881036).

We don't need this. Abandoning.

This is really useful. Thanks for doing this. I got a couple of questions and comments in-line.