- User Since
- Jul 7 2012, 2:54 PM (250 w, 2 d)
First off, really sorry to keep sending *partial* code reviews. =[ I again didn't quite have enough time to do a full review of the patch (it is a bit large) but wanted to at least send out everything I have so that you aren't blocked waiting on me to produce some comments. =] I'll try again tomorrow to make more progress here although it may start to make sense for me to wait for an iteration as one of the refactorings I'm suggesting here will I think change the structure quite a bit.
Sun, Apr 23
Fri, Apr 21
I'm still working on this, but since Wei mentioned he is looking at fixing the CandidatesToErase stuff, I wanted to go ahead and send these comments -- there is a significant one w.r.t. to the deletion stuff as well.
Digging into the code next, but wanted to send some comments just on terminology and the documentation while I'm doing that.
LGTM with some loop simplifications.
Thu, Apr 20
Restricting addrspace cast in this way seems... really hard to get right. I still have the question Eli asked: what does this mean in the absence of inbounds? What if the address spaces have different wrapping behavior even though they have the same number of bits?
LGTM as well -- no sense doing anything with the unreachable blocks.
Yeah, still LGTM. =D Tiny nit pick though...
Thanks for the updated patch description! Another minor request here.
Wed, Apr 19
Generally this looks pretty awesome. Thanks for all of this. Minor nitpicks inline. I'd let Craig have a look as well just in case I'm not seeing anything, but good to submit if he's happy as well.
Tue, Apr 18
Fri, Apr 14
Sorry I couldn't get back to it sooner, first chance I had.
Thu, Apr 13
Wed, Apr 12
Thanks for the review. Mostly done, landing with those updates. Couple of things left, but you indicated they could be follow-ups. Questions on exactly what to do with them below.
Tue, Apr 11
FWIW, I'm pretty happy with this approach especially for something triggered by an attribute like this. It is a bit gross, but doesn't seem *that* gross. Still, should see if your explanation satisfies davide. =]
After looking at the uses of AA in both passes here, I think I like the alternative idea of making AA optional directly in the passes. Then, for the old PM, we can use something like getAnalysisIfAvailable and just not mark the analysis as required when not optimizing codegen. What do you think? Would it be helpful if I hack up a patch?
LGTM. Also, this is clearly correct, feel free to land this simple of bug fix directly. =]
Mon, Apr 10
This really looks like it is going in the right direction. I'm going to work on reviewing some of teh code changes a bit more closely, but I wanted to mention one other thing.
LGTM from me, and thanks for the update around dyn_cast. Maybe wait for Mehdi or some second pair of eyes just to be sure I'm not missing anything.
LGTM as well.
LGTM, seems like a clear improvement in this code. See one nit about the asserts below, but feel free to submit with that addressed.
Thu, Apr 6
Rebase, enhance with better testing of const iterators, fix a couple of minor
issues, and re-clang-format everything.
Wed, Apr 5
First off, thanks for the new approach. I like this direction a lot. Some more tactical comments here.
Mon, Apr 3
Fri, Mar 31
This looks good to me but I'd like Hal to also have a look to make sure I'm not missing anything.
Thu, Mar 30
Wed, Mar 29
I've chatted a bit to Thomas about this off the list and wanted to follow up here so that there wasn't radio silence.
Mon, Mar 27
Sun, Mar 26
Mar 25 2017
Thanks a lot for working on this, first round of comments!
Mar 24 2017
Mar 23 2017
The fix seems trivial and looks good. Please submit to at least fix the crasher.
Mar 22 2017
But we use this to send color codes to temporary files that capture output for build systems and such when -fcolor-diagnostics is explicitly provided? (As an example...)
Mar 21 2017
This seems like a totally nice direction, but I wonder if I could convince you to scope creep this a bit?
Mar 17 2017
Update with fixes suggested in review.
Function argument order fixed.
I think it'd be good to send a quick RFC / heads up email to the dev list about the refactorings you're planning here just so that folks know what the roadmap is. I'll review this in the mean time.
Mar 16 2017
LGTM with whatever you decide about the allocator stuff.
LGTM once split.
This seems like a very reasonable tradeoff and a clean patch. Especially reasonable given the subsequent improvements. Ship it.
Mar 12 2017
Mar 10 2017
Nice, I like it.
Currently, I really don't understand why this is the right approach...
Update to fix subtle bugs in handling arguments. Thanks to David for the review
comments that caused me to see the issue here.
Mar 9 2017
Mar 8 2017
Mar 7 2017
So, this didn't go to the list. I'd abandon it and create a fresh revision with llvm-commits CC'ed directly.
Mar 6 2017
In addition to the issue David pointed out, I don't understand the motivation yet.
We intentionally didn't use zero because that seems more of a degenerate test case. What problem are you actually trying to solve here?
Mar 3 2017
Mar 1 2017
Feb 28 2017
Feb 27 2017
LGTM as well. Lemme know if you'd like me to land.
Feb 26 2017
Feb 24 2017
Some initial (minor) comments. It'd also help I think to rebase this as some of the refactorings land.
There's nothing wrong with this code, but this is a refactoring that I think only makes sense in the context where you need it. As-is, it seems like unnecessarily more layers with no real benefit. Let's look at this in the end-state patch?