OK for revert to green.

LGTM, one nit :)

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

It's mostly that we try very hard to not have #if 0 blocks in code please :)

In D88827#3148452, @jhenderson wrote:

In D88827#3148315, @avl wrote:

@sbc100's response to the library division. I have a very slight preference for separate, but don't care enough about it to push for it, if others are opposed. Adding them as a reviewer in case it gets their attention.

My own preference is also to make ObjCopy to be separate library, though I also do not want to push it if others want opposite. I read this message https://reviews.llvm.org/D88827#2344900 and this https://reviews.llvm.org/D88827#2346399 and this https://reviews.llvm.org/D88827#2346567 responses as a consensus on making it to be part of Object library. If we do not have a consensus, I am open to discuss it further.

I changed my mind a bit since my early comment you linked, having gone cold on the review and now having come back to it. Here's my thinking: the object manipulation performed by the objcopy code is based on an internal Object class that has nothing to do with the object classes within the libObject library. By putting the two inside the same library, we risk confusion ("which type of Object do I need for this functionality?"). There are fundamental differences which would make reusing the libObject classes for the objcopy code less than ideal - it may not even be possible without a lot of work.

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

Thanks! No need to wait on future approval with that change :)

In D109837#3016868, @tstellar wrote:

In D109837#3016845, @echristo wrote:

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?

Thanks!

The main feature that is missing from config.guess is the vendor detection (e.g. RedHat, OpenSuse, Ubuntu etc.), so the changes needed in config.guess to support this would be similar to what we have in clang now: https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/Distro.cpp#L97 There may be other features missing too, but this is just the one I'm interested in.

I also think that in general it would be nice to remove config.guess, because its license has caused some confusion with users and contributors.

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?

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?

Hi Saleem,

In D106450#2960467, @xbolva00 wrote:

In D106450#2960400, @alexfh wrote:

In D106450#2956763, @RKSimon wrote:

In D106450#2956001, @alexfh wrote:

In D106450#2955083, @aeubanks wrote:

Given that we have a test case and multiple people have reported regressions, can we revert in the meantime?

Especially if there's no obvious and clear forward fix, I'd appreciate if you could unblock us by reverting for now. Thanks!

Except that regresses other benchmarks that I was addressing with this patch - bullet etc.

Is the regression fixed by this patch more serious than the one introduced by it? Is it clear on how to fix the new regression?

I believe Bullet > “two internal *micro*benchmarks”

In D104080#2835108, @MaskRay wrote:

You can mention that this does increase -fprofile-use= object file sizes a bit but probably don't matter.

For instrumentation PGO users, -fprofile-generate= object files are much larger than -fprofile-use=, so -fprofile-use= slightly size increases don't matter.
For sample PGO users, this may increase the peak object file sizes a bit, but doesn't matter as your measurement for clang-13 shows.

Test Plan:
ninja check all, manual rtesting

This should be deleted when committing.

The size went up from 107KB to 322KB, aggregate of all the input sections.

How large is your clang-13? ~100MiB? Then this increase looks neligible.

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