- User Since
- Aug 4 2014, 10:15 AM (175 w, 1 d)
Wed, Dec 6
The patch looks good to me.
Please address the few inline comments and commit.
Sep 8 2017
Jul 14 2017
Apr 18 2017
Apr 10 2017
Apr 3 2017
Mar 16 2017
Mar 9 2017
Mar 2 2017
Added check for HasMemmove.
Mar 1 2017
With the patch and compiling with "-mllvm -stats" the spec 2006:
Number of memcpy's formed from loop load+stores: 42
Number of memset's formed from loop stores: 1398
Number of memmove's formed from loop load+stores: 129
Feb 28 2017
The updated patch addresses the comments from Eli and Krzysztof.
Feb 25 2017
Changes to address comments from Eli and Chad.
Feb 21 2017
Feb 16 2017
Looks good to me.
Feb 15 2017
I think Aditya has addressed the review comments from David.
David, do you see something else in the current patch that requires changes?
Feb 6 2017
The change looks simple enough. Please commit.
Feb 1 2017
Thanks Taewook for fixing this!
The patch looks good to me.
I cannot approve the patch, and I would like someone more knowledgeable with debug info to have a look as well.
Jan 27 2017
I don't mind reverting this part of the patch for now to help users of lldb.
In the long run lldb should be fixed to accept the c++ construct.
Jan 20 2017
I suspect this is an error in lldb. Please try with gdb to confirm.
Jan 12 2017
The change looks good to me: I will let one of the maintainers of selectionDAG approve the patch.
Thanks for fixing this such that we can close PR31382 and commit again the gvn-hoist patch.
Dec 30 2016
Dec 20 2016
Fixed the problems that Gerolf has asked to be addressed, and reduced the number of changes.
Taking over this patch: I will update the patch addressing all the comments from Gerolf.
Dec 13 2016
In the past I have seen some improvements to function inlining due to hoisting happening before inlining.
Also there may be some sinking (in cfg-simplify) happening before we have a chance to hoist expressions.
Let's commit this, and I will report if I see perf degradations, in which case we may as well run hoisting before and after inlining.
Dec 12 2016
Dec 11 2016
Committed as r289399.
Dec 6 2016
Nov 29 2016
Added the unittest and the change to make it pass: InsertBinop() should not hoist divisions by zero.
Sanjoy, I think your previous patch has fixed one more place SCEVExpander::InsertBinop():
the second assert from your unittest is still failing with the current patch:
Updated patch with all suggestions from Sanjoy.
Nov 28 2016
@mclow.lists could you please have a last look at this patch: the change is for a performance improvement (20% uplift on a proprietary benchmark), and all the issues mentioned in the review have been addressed.
The existing synthetic benchmark shows an overall improvement:
The numbers are from an x86_64-linux machine: Intel(R) Core(TM) i7-4790K CPU @ 4.00GHz
Overall the patch is a win on the synthetic benchmark.
We have also seen this in a proprietary benchmark where it accounted for about 10% speedup.
Please also post the performance numbers from the added benchmarks with and without the change to the lib.
Let's also add a testcase and show the performance improvement.
Nov 8 2016
Nov 2 2016
Oct 31 2016
Ping: Eric, Marshall, could you please approve or comment on this patch?
Oct 28 2016
Other than removing that comment the patch looks good.
Oct 27 2016
Oct 26 2016
The patch also implements the idea that Marshall proposed in:
I have an idea; it involves a macro that is sometimes "inline" and
sometimes not, and changes when you're building the library vs. when you're
just including the headers.
Oct 24 2016
Oct 21 2016
Oct 20 2016
Oct 19 2016
This patch has been committed as https://reviews.llvm.org/rL218375
I like the idea that this patch is trying to address.
Oct 18 2016
Please fix the few formating issues.
The patch looks good otherwise.
Oct 17 2016
Oct 16 2016
Oct 14 2016
If I remember correctly, we pushed the fix after 3.9 was released.
Could you please explain the problem of requiring trunk LLVM to build trunk libcxx?
Updated patch fixes problems in adi and reg_detect.
All polybench pass with -O3 -ffp-contract=on and off on x86_64-linux.
Ok to commit?
This got approved in the past review.
Let's commit it now that the clang bug was fixed.
Thanks Aditya for keeping track of this.
Oct 13 2016
Oct 12 2016
Oct 11 2016
Complete patch implementing Proposal 2 for Polybench.
The way diff and phabricator displays the MemorySSA.cpp change is strange:
all I did is move getLoadReorderability() and instructionClobbersQuery() up in the llvm namespace, all the rest remains in place.
As the amount of text remaining in place is less than what moves, diff decided to display the complement.
Updated to use MemorySSA.cpp:instructionClobbersQuery().
Thanks Danny for insisting on properly fixing this.
Oct 10 2016
Oct 7 2016
No. Not in the test-suite. We do not own that code, and I will not run clang-format on all the tests in the test-suite.