- User Since
- Apr 27 2015, 11:17 AM (208 w, 2 d)
Thu, Apr 18
Wed, Apr 17
LGTM with a minor fix needed below.
Tue, Apr 16
Sorry for the delay, just catching up after a vacation.
Mon, Apr 8
What part of the patch caused the need to change the internalize action to do promote+internalize in one go?
Fri, Apr 5
Just noticed something else looking through it again, question below.
I have mostly small suggestions below. Overall, it looks pretty good and straightforward.
Thu, Apr 4
Wed, Apr 3
Tue, Apr 2
Fri, Mar 29
Hi Caroline, I was looking at this patch as I needed to reference it in another discussion, and realized there was no test committed with the patch. Looks like old review comments exist for a test/ThinLTO/X86/builtin-nostrip.ll on a prior version of the patch, but that seems to have disappeared on the version that got committed (probably just a mistake during the commit process since it was a new file). Can you grab that test off the prior diff and commit it now?
Wed, Mar 27
Added a gold test so that we can test the native object case too.
Tue, Mar 26
This change caused a compile time regression (that I referenced at the end of my summary in D59696). In this case, there are huge numbers of select instructions. After this change, since we now update the ModifiedDT correctly, the loop over the function in CodeGenPrepare::runOnFunction will break after each select is optimized, due to ModifiedDT being set. As mentioned in D59696, even after making the DT build lazy, there is still a large regression because of the number of times we re-walk the function.
Mar 25 2019
Mar 24 2019
Address comments, sync in NFC change r356857.
Mar 22 2019
Mar 19 2019
Sorry missed your earlier update. LGTM. Thanks for doing the measurements!
Committed icmp test in r356463.
Mar 18 2019
Mar 15 2019
Mar 14 2019
Mar 13 2019
Mar 12 2019
Mar 11 2019
Mar 8 2019
Just got a report of an internal test failure from my commit, let me see if your patch fixes it.
Thanks for the fix. A couple comments below.
Mar 6 2019
Mar 5 2019
Feb 24 2019
Feb 16 2019
LGTM except for place noted below where I disagree with a change made earlier. Will let @davidxl chime in if he disagrees with me or has any other comments.
LGTM, but wait for @davidxl to do final approval since I'm not sure his last comments addressed.
Needs a test.
Feb 14 2019
Decided to take a different approach for now.