- User Since
- Nov 12 2014, 1:58 PM (140 w, 2 d)
Thanks, I'll review this soon.
A quick question. a concern people have about MemorySSA is that you pay upfront for O(1) queries, which may have some impact on compile time.
(e.g. in EarlyCSE). AST is already O(N^2), and in some cases badly so, but I wonder if you have compile time measurements of the impact of your change?
Thu, Jul 20
Trying to catch up on reviews.
Chandler, was this ever committed?
Wed, Jul 19
LGTM, nice to have you back from the grave :)
Addressed your comments & committed.
Do you know whether LTO gets this case right?
@anemet, this should be ready for review.
If I understand the patch correctly, aren't we just iterating children->parent, therefore the order of siblings should be irrelevant in this case ?
(i.e. the worklist is sorted according to the dominance relation and shouldn't really matter if we process two siblings in the dominator in different order?)
I think Sanjoy's suggestion works if my reasoning is correct, although I'm also fine with the current version of the patch (I don't have a strong preference for one or the other).
Please add a test exercising this codepath (you can take inspiration from the ones in GVN or LV)
Tue, Jul 18
The idea is sound, I'll think a little bit more about the visitation order (tomorrow morning) but it seems you got it right.
Just a minor nit, I see this patch has some unrelated formatting, mind to revert that? (and/or commit separately)
The logic is fine. Thanks!
Mon, Jul 17
I'm a little fried at the moment, so just some drive-by comments. I wonder if we can make the memory management automatic instead of calling naked delete.
LGTM with the small nits addressed.
Sun, Jul 16
I'm with Chandler on this one.
Fri, Jul 14
This change has no test associated. Can you please add one?
Also, this patch is oddly formatted, can you please run clang-format on it?
This fix is basically correct, I'm fine with this going in after Dan's comments are addressed.
I don't have strong opinions on the alternative solution suggested by Vedant, so up to you.
Thu, Jul 13
Forgot to mention. This fixes https://bugs.llvm.org/show_bug.cgi?id=33765
Looks good to me as well (agree that the machinecombiner bit should be separated, you can do that without pre-commit review, it's a no-brainer [assuming it doesn't break anything :)]
Wed, Jul 12
This is not your fault but it would be awesome if we introduced a common error handling for the dom, so that we can just call that routine which also calls flush.
(that way we don't have to repeat flush all over the place). Maybe in a separate patch.
Tue, Jul 11
This is good for portability but probably needs to be specified somewhere (programmer's manual?) but I'm not sure if we have a section on how to write tests.
Also, is there a netbsd bot to catch mistakes in future? [I don't see it].
Mon, Jul 10
Sun, Jul 9
Fri, Jul 7
Please hold the press on this one for a bit.
I didn't see @chandlerc / @dblaikie comments on the RFC and they seem to not have been fully addressed.
I guess you may want to continue the discussion there before coming back to this.
This is correct, but why?
Teresa, any further comments or we can go ahead and commit Charles' patch?
Thu, Jul 6
Thanks for the comments, Peter & Teresa. New version available.
Actually, I now think I understand this slightly better.
As this was handling only the update of linkage to available_externally, I think the easiest solution is that of moving the early return later and in case the original and the linkage in the summary don't match, and the one in the summary is weak, just switch to it.
I think this is safe, but maybe we might want to do the switch only for some kind of (original) linkages, maybe.
This has a known problem, I'll upload a new version in a bit.
I'm okay with or without the llvm-prefix.
I think it's useful for tools that have a GNU equivalent, but this one hasn't one, so, LGTM.
Wed, Jul 5
Updated after Eli's feedback. I'll change analyzeGlobals to not revisit instructions as a follow up if you're okay with it.
Thanks for taking the time to review this change.