- User Since
- Jul 7 2012, 2:54 PM (254 w, 4 d)
Thanks, landing this to fix the immediate issue (with comments addressed).
Add David to the reviewer set in case he can look sooner. I'd love to land this and unblock testing of the new PM + PGO.
Forgot to mention, I also added a test to verify that GlobalDCE won't remove available_externally functions that are actually in use in the module so it won't impair subsequent optimization passes.
FYI, I'm still waiting to hear back from you Danny...
Drive by comment...
What error are you trying to fix? We use flags without .getValue() all over the place, I don't know why we would need to change that here.
Mon, May 22
Fix comment even harder.
Update the comments to be more accurate and re-arrange the FIXME.
(FWIW, given the overall plan, I'm very happy for Hans to handle the code review.)
Sun, May 21
There is a lot of discussion here that I really don't think should be on a patch review. It should be an an llvm-dev thread. See below.
Sat, May 20
Another high-level comment: I don't like the name isoneof. To me, that parses to the relationship being tested is rather than isa which loses an important distinction. I'm not really sure what would be the best name though.
I'd like to request that you make this patch just introduce the tool, and maybe add a few usages. I think rewriting this much in a single patch seems like a bad idea. It'll make reverts if anything goes wrong pointlessly hard, etc.
Fri, May 19
Thu, May 18
To get more feedback, you might want to send an llvm-dev email with the benchmark data you have and ask others to benchmark by passing the flag there? More folks would see that email, and you can point at this patch as a place to have detailed discussion.
Wed, May 17
So, I think this was actually working as intended.
I definitely want this. First question is where?
Ahh, "atomic" max. I remember when I first wrote this in some other project 8 years ago now and had to figure out if it really has adequate forward progress guarantees... =] (It does.)
LGTM with some nits! Totally awesome.
LGTM with the assert changes.
Tue, May 16
Have you checked Polly and Clang for changes needed there? Detailed comments below.
Very nice generally. Comments only on one, but clearly apply to each variation.
LGTM again! =D
(Just marking this as no longer ready to go so it shows back up in my queue...)
Cool! This seems to me like a really nice simplification given the fact that the LPM has significant constraints on how you construct passes.
This at least seems like a very reasonable patch to me. I'd like to double check with David and Duncan as well (I've added them as reviewers). My only lingering concern would be if this will run the risk of underflow coming up more often, but that is probably not too much of an issue now that BFI does a better job of things.
Fri, May 12
Ping... Seems like this should be a simple patch?
It would be good in the patch description to maybe clarify the intent / motivation here? I assume the goal is to simplify reasoning about which thresholds are triggered in which cases when the new PM is available? At least, that is my guess based on our in-person conversations.
Thu, May 11
Update based on review comments.
Thanks both for the review. Landing with some fixes, but further discussion below and I'll follow-up with anything else.
High level comment: this feels really strange to be in instcombine.
This seems like a strict improvement to me. I'd love to have a more thorough analysis of different values here but I don't see any reason to hold up bringing this in line with the cold callsite threshold. LGTM.
Wed, May 10
Trying to provide answers to the open questions here...
LGTM on the LLVM side. Feel free to submit when you and Rui are happy with the usage pattern for this in LLD.
Tue, May 9
Forgot about polly which was broken by this. I fixed it in r302618.
Maybe I'm stuck with an old diff or something?
Other than a YAGNI argument against the type-erased stuff, seems fine to me.
Somewhat focused on the store side. Near the bottom is a high-level comment about the load shrinking approach.
Mon, May 8
You are continuing to argue that you do not need to add tests because they will pass. That seems to be missing the point. Tests are what verify and validate these kinds of arguments and assumptions, both now and in the future.
I don't see what would have addressed my concerns.
Fri, May 5
Thu, May 4
Generally makes sense. A question about the name. Feel free to land with a name that you and other sanitizer folks are happy with.
So, my concern about reviewing this isn't just about the implementation, it's also about the *API*. We need to make sure that the API design for parallel building blocks that we want to be available for the entire LLVM project are right. That's the particular split I was looking for...
So, I understand that this is just moving code from LLD to LLVM, but this is pretty complex and subtle code. I think it needs really careful and thorough review. I'm going to try to plan some time for that, but I wonder -- are there any meaningful splits you can make to introduce this more incrementally to LLVM
Wed, May 3
LGTM, excited! (But if you can, maybe wait for davide to confirm this is what he was looking for?)
Tue, May 2
LGTM with David's suggested change.
Mon, May 1
LGTM, this looks like a nice simplification and frees up the useful bit to support non-tracking VHs. Tiny nit below, but feel free to submit this and the non-tracking weak stuff when you're ready.
Sun, Apr 30
This seems trivially correct. Feel free to submit. If there is a use case, we can always add this back equally trivially when the code to use it is added...
relatively minor stuff here
(just marking this as needing changes so it isn't on my phab dashboard)
This test case should be cleaned up and merged into SROA/address-spaces.ll. Also, if I delete the 'getPointerAddressSpace()` call on line 3701, no test fails, so we clearly need better tests for the address space manipulation code in this file, or the call on line 3701 is actually dead and it should be *replaced* with the load-based address space in this patch.
Regardless of what you do with the code (I'm not an expert on this pass or the stuff Danny is already discussing there), please simplify this test case to a more readable and clear test case for the fundamental issue you're hitting. Having tons of Clang-specific bits in here really opacifies what scenario you're trying to test.
Fri, Apr 28
This seems correct and safe. Happy for you to land as-is, or to try a different approach if you want. I don't think you're making anything worse, that's for sure. =D
Thu, Apr 27
Thanks for the reviews, working on follow ups. Some are obvious and I'll just submit but let me know if you see anything weird.