- User Since
- Jul 7 2012, 2:54 PM (259 w, 1 d)
I landed some simpler refactorings in r306267 that should be enough to implement this.
FYI, I started looking at this. I think a somewhat substantially different refactoring of the instcombine API makes this much simpler by *only* extracting the logic for nonnull and range propagation rather than extracting all of it.
Tried to address the test comment -- let me know if something else was intended.
Update based on review comments.
Thu, Jun 22
Landed with the typo fix, thansk for the review!
LGTM, but please update description when committing to explain that you're also leveraging the facade to flesh out the iterator operations and removing the now unecessary manual implementations.
FWIW, I'm not really a fan of this behavior in the inliner.
Wed, Jun 21
I thought I had been clear, sorry if not. This LGTM based on the llvm-dev discussion, please go ahead. =]
Sure, comments inline...
Wed, Jun 14
Rebase and ping.
This makes lots of sense to me. Honestly, fixing the multiplies might be worth it -- if you can benchmark >1% improvement, I might do it. I'm thinking about the architectures where integer multiplies are *very* slow (atom at least, also some ARM chips)
Tue, Jun 13
LGTM. Some comments below about the name of the library and getting the linking and other stuff set up correctly, but I'm happy for you to sort that out and land (and fix bots as necessary) w/o going through more rounds of review. =D
Rather than using the RNG engine directly and using mod operations, the correct usage pattern is to use a distribution over the desired range and let it pull raw entropy out of the engine while mapping it into whatever distribution the use case requires.
Mon, Jun 12
FYI, I chatted with Zach in person to understand why the directory layout was as complicated as it was. I really wanted to simplify it. However, my initial attempts broke cross-project usage as Richard Smith pointed out, and this is *exactly* the kind of thing that we will really want to share across LLVM projects: Both LLD and potentially Clang will want access to things like the error matching utilities for their unittests.
Also, FYI, I agree with Krystof, updating the benchmarks is general goodness. We should be measuring against what Halide cares about, not some historical baseline.
Fri, Jun 9
Thu, Jun 8
I'm pretty happy to start simple with a flag and go from there.
Wed, Jun 7
I forgot to mention this: this needs a test case that would fail/crash before the patch. =]
What Andrew said.
Tue, Jun 6
I wrote this, but also have no memory of the intent. I'm pretty sure I was worried about something that could not in fact happen. I'm happy moving to an assert to check that in fact it does not happen. =]
LGTM, ship it! (and watch aall the world burn!)
Generally makes sense, but maybe check the intuition of some other folks who've been hacking on static heuristics recently... Also, a small simplifying (I hope) comment inline.
Added more tests, PTAL?
Add test coverage for the case-insensitive category logic.
Your testing strategy looks good. Comments below. I think this is getting close enough that it makes sense to start hacking on tests for it. Thanks so much for all the work here, I really like how this is turning out!
Mon, Jun 5
Reverted in r304762 and added another test case in r304763.
Since the name discussion seems winding down, review on the code changes themselves...
Folks, I know this has gone back and forth a few times, but I'm afraid I have to revert it again.
Fri, Jun 2
About the only thing I find questionable here is the Magic.h bit... And really only because it also has file magic for archives and bitcode. But that's probably OK.
Thu, Jun 1
Cool! I like the replay logic. It's a little alarming that we will replay arbitrary number of operations on an arbitrary number of phi inputs, but I'm *really* not worried here and the simplicity and clarity of the logic resulting from simple replay is great.
Generally this is starting to get pretty close. What are your thoughts on testing? I would suggest a unittest because that should be a good way to simulate an "out of tree" user.
Do you want to add a test to the defaults file that enables both? I don't feel strongly while they're off-by-default. LGTM either way.
LGTM, good catch!
This patch LGTM whenever the underlying LLVM change lands, thanks!!!!
(FWIW, I clearly like this patch as I sat with you. Mostly interested in tho folks who have worked on this library confirming they're happy with the API direction.)
some of minor nits a a small thing left... thanks for the handling and the nice test cases around larger integers!
First round of high-level feedback on this approach, thanks so much for working no this!
Ok, this seems like a trivial fix to a breakage, but please spell out in the patch description when you commit it what we discussed on IRC:
Thanks, landing with suggested fixes. (I also confirmed with David and Mehdi that they were happy.)
Honestly, the more I think about this, the more I think that the complexity it involves doesn't pull its weight.
Wed, May 31
LGTM, much nicer API now thanks!
Sorry, previous comment should have said LGTM with the tweak suggested.
Still not really happy with htis API, but let's fix it in a follow-up as it will grow the scope and much of the problem was already there (and the API is fairly narrow in scope).
Ping? I think I've addressed all the comments... Other patches are kinda blocked on this...
Tue, May 30
The missing initialize calls seem good to fix as-is, so LGTM for that.
May 26 2017
(FYI, I think this is ready for another look, only outstanding issue I'm aware of is Mehdi's concern around 'pre-link' terminology and that really just needs anyone else with naming thoughts to chime in...)
May 25 2017
Thanks for all the comments Dave! I'm gonna land this with the fix below, but do let me know if you want any further work here.
Address comments from both Teresa and David. Moved PartialIniling to run both
before and after thin link along with a FIXME to revisit the correct placement
of this pass long term.
(focusing on the LLVM side of this review for now)
I patched this and re-worked the particular API it uses for exposing the ThinLTO logic into a different surface in https://reviews.llvm.org/D33540 as an alternative approach. Thoughts?
May 24 2017
Thanks, landing this to fix the immediate issue (with comments addressed).