In D49532#1169016, @dblaikie wrote:

Probably squirrel it away in the generic 'flags' bucket?

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!

In D46230#1166958, @gaijiading wrote:

In D46230#1166919, @echristo wrote:

LGTM.

-eric

Hi Eric, I do not have commit access to trunk. Could you commit the change for me? Thanks.

LGTM.

In D49148#1166859, @echristo wrote:

In D49148#1166429, @ABataev wrote:

In D49148#1165826, @echristo wrote:

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

1. I can split it, no problems.
2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

Let me try to elaborate a bit, I agree that I'm not very clear above.

I'm not a fan of the generic "non default debug options". It feels misleading. I think we probably want to separate it by "dwarf extensions", and "dwarf version".

As far as the error message itself: "debug option" sounds like an option to debug clang rather than a debug information option. Perhaps say "debug information option" rather than "debug option"?

It's usually preferred to submit patches with full context - it'll make this a bit easier if you do please?

In D49148#1166429, @ABataev wrote:

In D49148#1165826, @echristo wrote:

I think you should break it out on an option by option basis. Just warning on "non-standard" options won't make as much sense to end users. Perhaps a "this option is unsupported on the target you're compiling for" etc.

Thoughts?

1. I can split it, no problems.
2. Hmm, actually this what the warning already says. If the option is not supported it says 'debug option '-xxx' is not supported for target 'xxx-xxx-xxx''. It does not seem to me like a warning on non-standard option.

In D49434#1165381, @pcc wrote:

I don't think that globalopt is the right place for this because there is no guarantee that the pass is run. If we wanted to do something about this, it seems like a better place would be in lib/Analysis/ModuleSummaryAnalysis.cpp where we decide which functions are GC roots.

Also this is only a subset of the functions that the code generator can generate calls to. There is a list of runtime functions in llvm/include/llvm/CodeGen/RuntimeLibcalls.def, but as far as I know it isn't 100% comprehensive. Using that list would probably be better than just 3 functions though.

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.

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.

In D48964#1158338, @ColinFinck wrote:

In D48964#1158046, @echristo wrote:

Has ELFOSABI_HERMITCORE been officially allocated?

I'm not aware of any official entity managing these ELFOSABI numbers, and have neither found references to one at the Binutils and LLVM websites.

In D49043#1163285, @paulsemel wrote:

In D49043#1163274, @jhenderson wrote:

LGTM, with the caveat that I don't know for certain if that was the original issue, without looking at the failing job and what was going on.

If you need to make minor changes to get the build bots working, no need for further review from my point of view.

I just asked @echristo if he could give this patch a try on non-x86 architecture, to be sure it was the problem ! :)

LGTM. Small organizational thoughts, but that's it.

As a quick change this looks ok, some thoughts for cleanup:

This is obviously good to start and to iterate on.

LGTM, a couple of inline requests. One small and one maybe less so.

The change looks obvious, I think the test is fine too, but MIR test cases aren't fun to read. :)

Has ELFOSABI_HERMITCORE been officially allocated?

Looking pretty good at this point. I am still curious why we've got the scheduling change here...

I'm ok here. Ray?

Oh, sure :)

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.

LGTM.

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.

Good enough for me. I hadn't had time to look. Thanks Eli.

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.

One nit, otherwise LGTM.

Adding Justin as I'm more comfortable with him reviewing this than me :)

In D48090#1131204, @sfertile wrote:

In D48090#1130092, @ruiu wrote:

Can you give me a pointer to the spec describing these relaxations?

Yep, the abi has a section describing each of the tls relaxations: http://openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655241_61284.html

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
make sense?

One inline comment then OK.

In D45265#1128965, @nemanjai wrote:

LGTM. I think we should go ahead with this.

I agree with the additional work Eric suggested as well. I believe we already use the MachineScheduler on all recent power cores as well as the BG cores, so we might as well make it an across-the-board setting and simplify the code.

LGTM.

LGTM.

LGTM.

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.

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.

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.

No strong opinions. I'm probably not the right person to review this :)

In D46791#1107168, @pcc wrote:

There were a bunch of them but the last one was D47093 which has already landed :)

Probably all that needs to happen on this change is to replace the isLinux() check with isELF().