- User Since
- Oct 15 2012, 2:12 PM (453 w, 5 h)
Thu, Jun 17
As part of the group reviewing here's an explicit ack. Happy to do post commit review on the rest.
Wed, May 26
I'm also not a fan of this change. From a project perspective the binary is clang and while people may wish to change the name for their own product teams it seems like that onus should be on them.
Mon, May 24
Not clear why we're going with not updating in place? Or just trying to do this in multiple stages?
May 21 2021
Given the iterations here I think that it needs a lot of inline documentation for cost model of what you're doing and what it's expecting here.
May 19 2021
May 4 2021
Couple cleanups, but should be better now. Thanks!
Apr 30 2021
I think Sam or Jonathan would be good reviewers here.
Apr 28 2021
Looks like they said with that change it was good for them and from my reading the IR is pretty good for a test. :)
Apr 27 2021
Sounds good. It's a soft objection, mostly because if nothing else it puts
us back where we were subject to some latent bugs, but perhaps not as bad
as before (though I don't find having to use an assert build reassuring ;)
Apr 26 2021
Apr 20 2021
You should error rather than xfail if you can.
I guess my question is "why? what does this make easier/harder/etc?"
Apr 16 2021
Apr 15 2021
Apr 10 2021
Apr 6 2021
Apr 2 2021
Mar 29 2021
Mar 23 2021
Mar 17 2021
Mar 16 2021
Mar 15 2021
I would prefer you use a checked in binary to test. I think it's reasonable to create a test case based on the dwarf emission support once you're convinced that's correct and go from there. For updates you can specify what the testcase is as a comment in the check. This is similar to a number of other tests FWIW :)
Mar 11 2021
So, why these locations rather than somewhere else? (Also lint checks/testcase/etc).
Mar 4 2021
In addition to the bikeshed below, I'm not a huge fan of this in general. I think we should assume that the libc we link is complete and thus it would just be named "libc.<ext>" and in a sysroot somewhere. Another option is perhaps looking at the rtlib option, but I'd want to see a bit of a writeup there so we can see what we'd be doing.
Feb 25 2021
Exclusion list? or exclude?
Feb 23 2021
Feb 22 2021
Feb 18 2021
Feb 8 2021
Feb 2 2021
Adding Tom here as release manager so they know.
Jan 27 2021
Jan 21 2021
Jan 20 2021
Jan 19 2021
Hard to tell because there aren't any comments around any of the code that changed in either patch. Can you fix that in a separate change and actually document what everything is doing? :)
I think this would be more clear as a combined patch rather than 3 separate and some reverted patches. Would it make more sense to revert the chain and put them all back together in a single review?
Is the summary correct? The patch that this is fixing isn't reverted...
Jan 17 2021
I'd like to avoid the bool completely if possible. Passing the register
might work as well if you wanted to do that, but I think it needs to be
clear API wise why and how you're doing computation where you are.
Sorry to point this out now, but taking a bool in/out pointer is some
fairly awkward API. Can we do something else here?
Jan 15 2021
Can you provide a little more detail on the motivation here? Thanks!
I'm not quite sure I understand. What's the issue between xcoff and emission here? Does it not support subtraction? It's been a while and I can't recall.
Jan 13 2021
If you changed the existing patch it won't be clear that this is something that needs review. Can you make a new review with the bug fix?
Jan 5 2021
Dec 4 2020
Dec 1 2020
Thanks for the explanation, lgtm.
I think Florian's diagnosis is worth looking into for sure - and probably
is the right solution. For the regressions I had they could largely be
explained by the branch predictor on a micro level, I didn't see some of
the large scale LTO level regressions in mine.
Nov 20 2020
Why is this useful rather than having targets set the address space used in the backend explicitly? i.e. how is what amdgpu was doing up to this point not ok? It's not really clear here.
Nov 19 2020
Nov 16 2020
I think this is reasonable, if neither George or Peter say anything I think this looks ok to me.
Nov 13 2020
Also probably a good idea :)
I'd just seen that too. Thanks!
Nov 7 2020
This looks fine to me for now. I really think it's too wordy and too elaborate and could be about 1/10th as long.
Nov 5 2020
For the future you can just commit this sort of thing. Though I admit to being curious about the lint error :)
Oct 29 2020
Let's go ahead and unblock you, but getting a lot of this refactored would be great if you can. I think it's hitting the limits of the original design. :)
Oct 28 2020
I'll take a look tomorrow, sorry for the delay.
I'm going to accept this as it codifies a lot of existing intent behind how the community works. This was also fairly universally accepted 4 years ago in the original discussion and nothing has changed.
Oct 24 2020
Seems reasonable to me.
Oct 21 2020
I really do think this needs to be under object. It can be separate in there as lib/Object/Copy if you want, but I don't think it should be a parallel directory. Hopefully the updates for the move aren't overly onerous here.
Oct 14 2020
Awesome. Thanks for the update :)
Oct 13 2020
Sorry, didn't see this last time. Couple of inline comments, mostly nits.
Mark as requesting changes :)
Oct 12 2020
Oh, you're waiting on me. Uh, I'll get to this tomorrow. :)
Oct 9 2020
I like the direction this is going, I'll take more of a deep look soon, but wanted to ask: "should this be in Object rather than a separate library?" When I'd originally asked for this to be split into its own library I'd thought that it would get added into libobject.
Oct 8 2020
Arguably since this is going to be very cpu dependent perhaps it should be
part of TTI?
Oct 2 2020
Drive by as it's years since I've dealt with any of this:
Sep 18 2020
Sep 17 2020
Sep 16 2020
Sep 2 2020
I don't think your statement matches what I was seeing. I may have
constructed the testcase poorly, but there were differences being shown.
Also keep in mind that your clang run is testing what I've already fixed. :)
Sep 1 2020
I think this is the right thing to do here. In addition, we should not cherry pick this to any release branch so we have the opportunity to get it more fully evaluated by distros/vendors/etc over the next months.