- User Since
- Oct 15 2012, 2:12 PM (506 w, 2 d)
Fri, Jun 3
OK for revert to green.
Apr 18 2022
Mar 7 2022
LGTM, one nit :)
Feb 24 2022
My biggest concern is llvm developing a gcc-esque "--params" style set of options which is what this would give. If we want these to be a support surface we should enable and test them as such. Just allowing it without that gives the entire ecosystem another set of support surface that we actually don't want people to use. Workarounds downstream should be handled by downstream as much as possible or via coordination with developers on a "hey, can we actually wait here" on options.
I'd probably prefer that we don't do this since we're looking primarily at options that shouldn't be used outside of development, e.g. clang has a driver that does accept these kinds of things as does lld. Open to arguments for and against though :)
Feb 23 2022
Jan 24 2022
It's mostly that we try very hard to not have #if 0 blocks in code please :)
Jan 13 2022
Dec 20 2021
Nov 22 2021
Nov 5 2021
I think you are these days too :) My offer was "past-past-me". I think
you're probably ok here, I did a rough scan, but getting someone like Aaron
for the attribute support would be good.
FWIW I'm a bit rusty in this area myself, but thanks for doing this. Let's see if we can't get Aaron to continue reviewing :)
Sep 23 2021
Thanks! No need to wait on future approval with that change :)
Sep 22 2021
This systems seems somewhat more fragile than config.guess? I guess I'm just not convinced that doing this versus the straightforward changes to config.guess are worth our effort? Can we get an idea of what changes you don't want to make to the local copy of config.guess?
Sep 16 2021
I think I'd rather not tell people to download config.guess. We can still modify the one we have, have there been serious problems doing so?
Sep 10 2021
Sep 8 2021
Aug 23 2021
Jul 22 2021
Jul 16 2021
Jul 8 2021
Jun 23 2021
Jun 17 2021
As part of the group reviewing here's an explicit ack. Happy to do post commit review on the rest.
May 26 2021
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.
May 24 2021
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 :)