- User Since
- Dec 15 2015, 10:55 AM (182 w, 6 d)
Thu, Jun 6
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.
Thu, May 30
I think r320996 implements this.
Fri, May 24
Thu, May 23
Tue, May 21
May 16 2019
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.
May 8 2019
added symbols and check they are listed
May 3 2019
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).
May 2 2019
- 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
May 1 2019
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.
Apr 30 2019
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.
Apr 29 2019
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.
Apr 22 2019
Apr 18 2019
Does this look good now?
Apr 15 2019
Apr 11 2019
without second constructor
Apr 10 2019
Apr 9 2019
Updated as suggested by @rsmith (thanks!)
Apr 4 2019
Apr 2 2019
Feb 19 2019
Feb 14 2019
Feb 8 2019
Use [=] instead of explicitly passing StringRefs to ReportBufferError.
(Following the same archive(member) format also used by toString() in InputFiles.cpp)
lld-link: error: could not get the buffer for the member defining symbol spvDiagnosticDestroy: No such file or directory
Feb 7 2019
See earlier comment.
Feb 6 2019
Jan 30 2019
Jan 29 2019
LGTM, with some suggestions in the comments.
Jan 24 2019
Re-upload only the test change.
pcc, does D57192 implement your suggestion?
Just saw your comment, pcc. I'll look into that.
Jan 23 2019
add missing .1
replaced .c test with an .ll test
Jan 22 2019
Jan 4 2019
Sep 14 2018
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.
Sep 6 2018
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'.
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).
Aug 24 2018
We don't need this. Abandoning.
Aug 23 2018
This is really useful. Thanks for doing this. I got a couple of questions and comments in-line.