- User Since
- Apr 27 2015, 11:17 AM (246 w, 4 d)
LGTM. Thanks for the explanation.
Wed, Jan 15
One thing to consider changing/removing in the summary is this comment:
"For example, (enum) "Level > 1" captures not only O2 and O3, but also Os, and Oz."
since that is actually correct (Os/Oz should be included in Level>1 as they are O2).
Is this a bugfix and should there be a test case?
Tue, Jan 14
From an LTO perspective, this seems fine for the reasons we discussed here. I looked through the patch and have a few comments.
Overall I like this approach.
Can you re-upload the latest with context? It isn't available and therefore I can't compare the last two diffs.
Mon, Jan 13
LGTM. One more request for a comment below that I forgot to add earlier.
I'm not sure if ThinLTOCodeGenerator.cpp and LTOBackend.cpp were intentionally left out due to the LTO concerns mentioned in the description?
Sorry for the delay, I lost track of this one.
Fri, Jan 10
Thanks for fixing this missing -Rpass support!
I just have a few high level comments from looking through it just now. The summary needs a fix since Os/Oz are in fact O2 so OptLevel > 1 was not doing the wrong thing.
Remove some cruft
Thu, Jan 9
A few minor suggestions for comment tweaks, but lgtm otherwise.
Thanks! Looks pretty good, I just have a few comments and questions.
Wed, Jan 8
Thinking through this some more - this patch fixes the distributed ThinLTO backend case, but not the in-process ThinLTO case. For that, both lld and gold plugin will need to set this up. And since they don't have the clang options, they will just need to set to some reasonable default, probably either hardwired on like in the old PM or set based on the CGOptLevel they set from linker options. However, they both default to -O2 unless a special plugin or lld linker option is set, so we'd probably want to enable these passes there at O2+.
I think the old PM has some issues as well, although more in the opposite direction. The code in LTOBackend.cpp:runOldPMPasses is simply hardcoding loop and slp vectorization flags to true, but presumably should use whatever is passed through the lto Config as well. Not sure about LoopsInterleaved, ah looks like it is set to the value of the internal option which is enabled by default. That and DisableUnrollLoops should also use whatever is passed through here (similar to how they are set in clang BackendUtil.cpp when configuring the old PM).
Tue, Jan 7
Mon, Jan 6
Sun, Dec 29
Attempt to fix patch to not include parent revision changes
Implement suggested name change.
Fri, Dec 27
Thu, Dec 26
Dec 17 2019
Dec 16 2019
Dec 13 2019
Please take a look. This is now updated to reflect the commit of D71193, which translated the options to the new attributes. I also removed some comments that I realized didn't make sense, as we need to keep a baseline availability array on the base TLII since that is set based on the architecture. We simply override this for no-builtin* attributes on the function. I removed the code from clang that was setting up the availability array based on the options, and all tests pass.
Update after D71193 committed, converting -fno-builtin* options to attributes.
Dec 12 2019
Dec 11 2019
This LGTM, although I have a couple of questions that are orthogonal to this patch and shouldn't block it. Please wait to see if @aaron.ballman has any more comments.
LGTM with a few small suggestions before you commit.
Dec 10 2019
It seems reasonable to support this from LTO since it provides analogous support to what is available via clang without LTO. @pcc what do you think?
Dec 6 2019
Dec 5 2019