- User Since
- Jul 7 2012, 2:54 PM (283 w, 3 d)
Fri, Dec 8
Thanks for all the feedback!
Update based on code review comments.
If MSan doesn't complain about these, I'm inclined to leave them uninitialized. I'm not sure how valuable it is to workaround valgrind false-positives.
Thu, Dec 7
Tue, Nov 28
Thanks for all the feedback, landing with suggested fixes.
Mon, Nov 20
Update to address the issue highlighted by David and add more test cases.
Fri, Nov 17
Wed, Nov 15
Thanks Davide, I think I've got these covered either by adjustments or future work. Give a shout about anything you'd still like addresed in follow-up patches.
Tue, Nov 14
LGTM, nice catch!
Mon, Nov 13
Thanks, landing with comments addressed.
Rather than adding llvm-commits to an old revision, please abandon the revision and create a fresh one with the patch and llvm-commits at the start. That way email gets sent to the list with the full description of the patch. Thanks.
Nov 9 2017
Nov 7 2017
Oct 26 2017
As with Davide, my argument was mostly "use an existing bucket" not "it should be in bucket X". =]
So, doing research to understand the impact of this has convinced me we *really* need to stop doing this. Multiple libraries are actually trying to enumerate every CPU that has feature X for some feature X. =[[[ This, combined with the fundamental pattern of defining a precise macro for the CPU, leaves a time bomb where anyone that passes a new CPU to -march using some older headers will incorrectly believe features aren't available on *newer* CPUs. =[
Oct 25 2017
I don't really like the ever growing set of configuration macros here.
Oct 19 2017
Oct 18 2017
Oct 17 2017
Add a test case covering critical edge splitting.
Add a stricter form of checking for profitability to avoid one potential issue
with this patch. Also clean up comments in various places to try and help
address code review feedback.
Hopefully responding to all of the comments. Note the last one which is probably the most interesting point of discussion around cost modeling.
Oct 16 2017
Update addressing the remaining comments from Sanjoy.
Updates and/or responses. Largely addressed all the comments with the next patch upload.
Oct 15 2017
Rebase onto the new LoopInfo API and get everything working. Also rebase into
a monorepo checkout.
Oct 13 2017
This patch got reverted again, and I wanted to provide some insight as to why...
Oct 12 2017
Oct 11 2017
Oct 10 2017
Have you considered building a ChunkedVector instead of a ChunkedList? Specifically, there is a great trick where you use a single index with the low bits being an index into the chunk and the high bits being an index into a vector of pointers. It has many of the benefits you list and is a bit simpler I think. It also supports essentially the entire vector API if desired. Both bi-directional and even random access are reasonably efficient. Good locality, etc.
Oct 7 2017
Oct 5 2017
(quick responses, still working on code updates from the review)
I've taken some time to go through the code in NewGVN that computes similar things based on the suggestion from Danny. These now do *very* similar things in terms of walk. The differences I've seen are:
Oct 4 2017
FWIW, I think with the latest patch update this has now addressed Danny's primary concerns, and should be much more effective at avoiding re-visiting regions of IR.
Rebase (and move to mono-repo layout, sorry for any disruption).
Oct 3 2017
Oct 2 2017
Is there any way that the debug instructions can become invalid due to this transform? I'm not seeing anything obvious, but wanted to make sure you've thought about this too.
I generally like the direction of the cleanup. A few high level questions:
I'm generally fine with this going in given the measurements.
Oct 1 2017
Sep 27 2017
Sep 26 2017
(Marking as accepted now that Easwaran has had a chance to look as well)
This is looking pretty good. Some nit-picky improvements below. I'd love for Easwaran to get another chance to look at this too though...
Sep 22 2017
Mostly minor suggestions below...
Sep 21 2017
Can you revert the unrelated formatting changes? Otherwise reviewing this is a bit hard.
Could you include analogous updates to the new PM's pipeline? (I can do it myself in a follow-up if you'd rather...)
Sep 20 2017
Rebase (no substantive changes yet...)
Sep 19 2017
Generally looks good. Description should likely be updated to reflect changes made during review. Feel free to submit!
I suspect Reid is in a much better position to look at debug info changes to SROA than I am, but let me know if I can help...
(marking as needing changes to clear dashboard)
Marking as needing changes to clear dashboard.
Just wanted to say thanks so much for working on this -- its a huge improvement to the inline cost.
Sorry for the awful delay when I got pulled into stuff, should be able to stay with the (small) stuff left in the review.
Generally like the API changes here. The enum seems a big improvement.
LGTM, nits below.
But, the verifier itself will just "crash". It won't print a stack trace, but I don't see why that's much better? And this flag is supposed to be a developer option and not a user facing one, so I'm somewhat confused at what the intent is here...