As part of the group reviewing here's an explicit ack. Happy to do post commit review on the rest.

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.

Not clear why we're going with not updating in place? Or just trying to do this in multiple stages?

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.

Couple cleanups, but should be better now. Thanks!

I think Sam or Jonathan would be good reviewers here.

Looks like they said with that change it was good for them and from my reading the IR is pretty good for a test. :)

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 ;)

In D99976#2706447, @nikic wrote:

In D99976#2703736, @aeubanks wrote:

This slightly regressed compile times while it was in
http://llvm-compile-time-tracker.com/compare.php?from=080d48f279e2b70e4e9fc65bc4947a53ffc982f3&to=d87b9b81ccb95217181ce75515c6c68bbb408ca4&stat=instructions

Interesting, I didn't expect that. Presumably this is because most places working with IntrinsicInst now have to perform two branches rather than one, and apparently it gets used a lot. Most likely this overestimates actual impact (assuming predicted branches), but still, better to avoid it if we can.

In D100776#2699603, @thakis wrote:

In D100776#2699565, @rnk wrote:

Of the three people who commented on D17183, you and I are on the only ones in favor of this approach. I think @echristo and @jyknight both preferred approach 2. I'd like to get at least an agreement to disagree from one of them before approving this.

That's 50% in people, and 15/24 voting by changes that landed in clang/ since start of year ;) Don't know why 2/4 is "only".

You should error rather than xfail if you can.

I guess my question is "why? what does this make easier/harder/etc?"

LGTM. Thanks.

SGTM. :)

In D17183#2656771, @jyknight wrote:

In general, I think it's extremely unfortunate that Clang and LLVM have different copies of the same information. It's a problem for way more than just this one situation. So, I really don't like choice 1 -- I think it's moving in the wrong direction.

The recent thread about compiler-rt libcalls vs size of "int" is another example of this sort of issue of duplicated info across the llvm/clang boundary being troublesome. Other times, the information in question is buried in the Target implementation in LLVM, and Clang doesn't depend upon targets being compiled in...usually, so it can't access it from there. E.g. the fact that Clang even has to come up with a DataLayout string on its own is an example of this problem.

So, I like option 2. I think a lot more of the information in TargetInfo ought to be shared with LLVM. I note that ARM already pushed a bunch of shared stuff into LLVM to reduce duplication, which is great. But, it had to put it into a rather odd place (llvm/include/llvm/Support/{ARMTargetParser.h,ARMAttributeParser.h,...), because of the layering and optionality concerns. That part is not so great -- putting all this target-specific info into "Support" doesn't feel like the best fit. There should be some sort of Target-specific location for this sort of stuff to live which Clang can depend on.

In D96881#2627389, @nikic wrote:

LGTM per above discussion. It's not ideal, but I think it's the best we can do right now.

In D97186#2631868, @shchenz wrote:

Thanks for review @echristo

a) Can you name it something other than empty? Maybe basic or something?

Done

b) Reasonable to loosen up the checks slightly? There's a lot of -NEXT in there that doesn't appear to be strictly necessary (unless you're autogenerating the checks - if so let me know).

Yeah, sort of auto-generating. But do we need to loosen up the checks? This DWARF info should be settled as the result of the tests comes from object files.

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 :)

So, why these locations rather than somewhere else? (Also lint checks/testcase/etc).

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.

Exclusion list? or exclude?

In D96035#2582780, @avl wrote:

In D96035#2582232, @JDevlieghere wrote:

In D96035#2579270, @dblaikie wrote:

A non-deterministic behavior is probably not acceptable - it'd certainly be no good at Google if we ever wanted to use this for DWARF linking. Not sure how the Apple folks feel about it for classic dsymutil.

Yes, this would be a non-starter.

even if it IS deterministic in --num-threads=1, and is Not deterministic in multi-thread case?

LGTM. Thanks!

In D96906#2572842, @msearles wrote:

In D96906#2572749, @kzhuravl wrote:

The point is that nobody upstream even got a chance to chime in.

We are and will be taking care of any feedback provided in this review post-commit.

To be fair to @rampitec , it was not his desire to push this up in 1 big patch. We needed this upstreamed and no time was given to him to break it up into reasonably sized pieces. If it appears to be his doing/his intent, well, it should not. There have been a couple comments; I believe most addressed; comments will continue to be addressed.

In D96203#2548495, @aaron.ballman wrote:

In D96203#2548471, @mibintc wrote:

In D96203#2546856, @aaron.ballman wrote:

Thank you for working on this!

This changes the option names that include substring blacklist to blocklist.

I think this change works in some places, but in other places we say things like "blocklisted" which feels a bit awkward. I don't super love the name blocklist, but I don't super hate it either. All the alternative names I come up with aren't really great either, like UnsanitiziedEntities or IgnoredObjects. Maybe someone will have a better idea here than me, but I do think the current name is an improvement over the old name.

There is a pre-existing patch https://reviews.llvm.org/D82244 using block and allow, that's why I chose blocklist. Although going in cold I would prefer to use allow and deny, and block seems to me a little like cheating (too similar to black).

That was my concern as well -- I can easily imagine someone seeing "blocklisted" and thinking it was a typo for "blacklisted" and undoing the change to comments with an NFC change.

However I can "disagree and commit" (cf. https://en.wikipedia.org/wiki/Disagree_and_commit) with this choice of wording. I can search out the awkward Blocklisted and come up with different phrasing in the prose, but I think names in the program should use blocklist so that they can be easily connected to the option name for future maintainers of llvm. I'll check this.

I agree that the frontend option names and the internal variable names need to connect to one another, so if we pick a new term, it should be used consistently. I'd be mildly happier with a better term than blocklist, but I can live with that name too.

Adding Tom here as release manager so they know.

Awesome. Thanks!

Hmm?

In D94972#2507400, @ABataev wrote:

In D94972#2507385, @echristo wrote:

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? :)

This is a separate patch with the fix only. :) There are no more patches required to fix the issue, just this one. Or do you mean something else?

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...

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?

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.

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?

In D91050#2480906, @steven.zhang wrote:

Ping...

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.

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.

I think this is reasonable, if neither George or Peter say anything I think this looks ok to me.

Also probably a good idea :)

I'd just seen that too. Thanks!

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.

For the future you can just commit this sort of thing. Though I admit to being curious about the lint error :)

In D89158#2363220, @aeubanks wrote:

Looking at BackendUtil.cpp in Clang as well as the Rust code, I'm back to thinking that we should provide a way to to all callbacks. Then in the case of passes added via TargetMachine, we should extend TargetMachine::registerPassBuilderCallbacks to also take a PassBuilder::OptimizationLevel. Then targets can skip adding optional optimization passes at -O0. Does that make sense? It would allow us to clean up Clang and Rust.

In fact I see in AMDGPUTargetMachine::adjustPassManager a check for an optimization level, so some targets are already doing this, although it looks like it's more for codegen and not IR passes.

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. :)

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.

Seems reasonable to me.

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.

Awesome. Thanks for the update :)

Sorry, didn't see this last time. Couple of inline comments, mostly nits.

Mark as requesting changes :)

Hi Peng,

Oh, you're waiting on me. Uh, I'll get to this tomorrow. :)

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.

Arguably since this is going to be very cpu dependent perhaps it should be
part of TTI?

Drive by as it's years since I've dealt with any of this:

:)

In rG549e55b3d5634870aa9d42135f51ad46a6a0e347#948006, @thakis wrote:

Which header do you think is missing? It built fine on several machines over here.

In D86485#2277714, @rnk wrote:

Here is the sequence of what happened as I understand it:

• D71687 is intended to fix the full loop unrolling pragma with the NPM at -O1. The accompanying clang test covers this.
• D71687 also adds an LLVM IR test. It is an integration test: it started from unoptimized IR and ran the full -O1 NPM pipeline. The IR happens to contain optnone. Maybe that was unintentional, since one used to be able to run clang foo.c -emit-llvm -o - | opt -O1 -S and get optimized IR. Now clang applies the optnone attribute, and that no longer works.
• D85578 made the IR test more targeted: now it only runs the full unrolling pass instead of the O1 pipeline.

There is value in integration tests, so maybe the thing to do is to bring back the IR integration test in addition to this unit test. The unit test seems correct to me. If optnone is present, I would not expect the loop unroll metadata to be honored.

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. :)

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.