- User Since
- Oct 15 2012, 2:12 PM (300 w, 3 d)
I don't necessarily think this should be part of DIBasicType since it'll be used almost never. Adding other reviewers to get thoughts on location.
Getting close, one inline comment.
LGTM. The comment is particularly helpful, thanks!
It's usually preferred to submit patches with full context - it'll make this a bit easier if you do please?
Concerned about why... but let's just do it for now and figure out the test problem later. It's not a problem with the code as far as I can tell.
Tue, Jul 17
LGTM. Looks like a nice set of API simplifications.
I think you should break it out on an option by option basis. Just warning on "non-standard" options doesn't really make sense.
You'll want to wait on Rui for an actual LGTM, but this looks good to me and will match the llvm support so we can try this out in more detail and see where we should move the proposals.
Checking out the archives for this list might come in handy:
LGTM. Not a fan of adding more stuff to TargetOptions, but for now it'll do.
LGTM. Thanks and sorry for the delay.
LGTM. Small organizational thoughts, but that's it.
Mon, Jul 16
As a quick change this looks ok, some thoughts for cleanup:
Thu, Jul 12
This is obviously good to start and to iterate on.
LGTM, a couple of inline requests. One small and one maybe less so.
Wed, Jul 11
The change looks obvious, I think the test is fine too, but MIR test cases aren't fun to read. :)
Tue, Jul 10
Has ELFOSABI_HERMITCORE been officially allocated?
Mon, Jul 9
Sun, Jul 1
Fri, Jun 29
Thu, Jun 28
Looking pretty good at this point. I am still curious why we've got the scheduling change here...
Wed, Jun 27
I'm ok here. Ray?
Oh, sure :)
Tue, Jun 26
Mon, Jun 25
I've added a couple of inline comments here - between this and the comments in the post-commit review from dlj it seems like we might want to revert this for now and figure out the best way forward.
Fri, Jun 22
Thu, Jun 21
Sorry about the late review. I've added a lot of comment and documentation requests for the code and a couple of questions.
I think this is obsolete after the other patch?
OK, this ranking looks good to me. Go ahead and put your long comment with the layout into your commit message and do fix up the tests.
Tue, Jun 19
Good enough for me. I hadn't had time to look. Thanks Eli.
Jun 19 2018
As a drive by comment: While I think that consumers should be able to handle this case (obviously the bfd/gold workarounds here aren't always effective), I do think that we should probably implement this as well so that the consumers don't have to deal with it in a lot of cases.
Seems reasonable and fairly obvious.
Jun 18 2018
Jun 14 2018
Jun 13 2018
Adding Justin as I'm more comfortable with him reviewing this than me :)
Jun 12 2018
One inline nit and adding Dave here.
The description you had is basically how and why this stuff works. Just
switch "older cpu" for "minimum gpu" and I think all of this should still
Jun 11 2018
One inline comment then OK.
Jun 7 2018
Jun 6 2018
One inline comment, but otherwise looks ok. You could also split out all of the non-bionic/skip_destructor_rounds into cleanup and other patches as well.
Some of the logic in the function feels awkward, but that's a cleanup for another time probably.
Looks like a simple oops and feel free to fix anything like this without putting up a review in the future :)
Right now I don't have a better design without quite a bit more in depth look. That said, this gets us through a correctness issue and can be revisited in the future so let's go with it.
May 30 2018
May 27 2018
It sounded like Hal wanted to review this and I don't know that any of the other people on the review line have any experience with loop passes and so probably shouldn't have been approving this. I might be wrong here, and if so I apologize, but it seems like this went in a bit early.
May 24 2018
Relatedly it might be a good idea to remove needsAggressiveScheduling and just update the few testcases. I don't think it's worth maintaining the difference for processors that haven't been released in the last decade.
You might want to wait for Nemanja to ack as well.
Oh, I see now. Sure.
May 23 2018
No strong opinions. I'm probably not the right person to review this :)