LGTM. Let Nemanja give the final approval.

This is great with me.

Let's go ahead with this one for now. Sam's patch can go on after if we decide to go with a C API way of accessing this rather than just inline assembly.

Acking to clear the no bit.

Looks totally reasonable to me. Going through and cleaning up the last set of bool flags when you get a chance would be nice.

Couple inline comments, first should be handled, otherwise looks OK to me.

The two I had were 0xf2666d and 0xf3666d that were broken with the earlier patch that I mentioned in my reversion email following up on the original.

Have the front end generate all of the correct target features for your stub then rather than adding them in the backend?

Which isn't necessarily going to be valid either - particularly in the face of LTO.

Not the way I'd have done this. What you probably need to do if adding a stub is propagate the features from the function you're cloning rather than constructing a new (and possibly incomplete) set of features from the target triple yourself.

Fine with me.

One inline comment, but go ahead and commit after fixing that up.

Seems reasonable to add a comment as to what microarch features we're attempting to target as modern here.

SGTM.

In D35569#816289, @rengolin wrote:

In D35569#816129, @fhahn wrote:

With FeatureARM, the generic CPU would have to have this feature, but when the features of the architecture (v6m) are applied, FeatureARM cannot be removed, so the behavior will be different to the current one (Architecture features are applied after CPU features I think). We could rename FeatureARM to FeatureThumbOnly, which is a bit clearer in my opinion.

Correct. FeatureNoARM is specific to M-class. I don't mind how we call it as long as it's explicit the intention, and I think both FeatureNoARM and FeatureThumbOnly are clear.

In general looks ok. Mind commenting the if(NeedLoad) areas as to what you're adding on and why? The symbol is (IMO) obvious, but the random bits less so.

One inline comment, otherwise lgtm.

I'm ok with this.

Yep.

LGTM.

Tagging in Matthias here.

In D35577#817267, @sgundapa wrote:

The discussion is scattered across these patches https://reviews.llvm.org/D35578 and https://reviews.llvm.org/D35579.
I will provide a brief summary here:

The idea is to control the generation of data (lookup table) generated from a function, specifically when the user is not expecting it.
For hexagon, there is tightly coupled memory and the customers usually place "text" in it.
For functions, which generate lookup tables, it is very very expensive to read the table from a far away non-TCM data section.
This option will disable the generation of lookup tables at the expense of code bloat. This is really driven by the customers of hexagon backend.

I'm going to say this ahead of time without looking into it "LGTM", but wait for ctopper (or someone else) to ack it for style etc since I'm unlikely to get to it any time shortly. :)

So, what's the overall logical idea behind the need for this option? While I understand that you don't want people just doing this via the -mllvm set of options, but if you're just doing this to provide a temporary option rather than turning it off in the backend I'm not sure what the point is.

LGTM.

LGTM

LGTM.

Seems reasonable...

<insert comment about offline discussion about the two lists unfortunately not syncing>

In D35572#813520, @erichkeane wrote:

In D35572#813369, @echristo wrote:

Can you update the code in CGBuiltin to use this as part of this patch? :)

Thanks!

I'm not sure which you're referring to? I don't see a bit in CGBuiltin that checks for a subset of the "Features", however it is a much smaller list (since it is only things that the compiler-rt can check). Is it perhaps more closely related to that one?

LGTM.

LGTM pending dependency approval

Can you update the code in CGBuiltin to use this as part of this patch? :)

From a high level it sounds fine. You'll probably want someone else that's hacked on cmake to give an ack though. :)

FWIW the duplication in CGCall.cpp of the enum set is painful if you can come up with anything else it'd be awesome. I don't have any good ideas, just a fond wish :)

From my perspective here once you and Erich get some agreement on the checking between your two bits I'm fine :)

Adding Erich Keane here on this since he's working on something similar for the target attribute.

If you wouldn't mind I'd like to see this separated out a bit -
a) I think we can make the attribute info a struct rather than a pair as a simple change first
b) setCPU split
c) let's see where we are after that?

This sort of thing seems obvious in the future :)

In D35287#807305, @efriedma wrote:

Having a default architecture is convenient for messing around... if we're going to do something here, I would rather just kill off "-march".

In D34622#806301, @fhahn wrote:

Thanks. There are people currently testing mixed ARM/Thumb LTO on real-world projects and I assume there will be a few more bugs that need addressing. I think it would be worth waiting a little bit with adding the release note until they had some more time for testing.

Cheers!

Not quite what I meant, but if you want to ensure that this crashes the compiler rather than emitting bad code then this is fine. Can you also add a TODO with "this should return an error" or some such?

Not a huge fan, but also don't have any better ideas for now.

OK.

Perhaps an explanatory comment? :)

In D35214#804024, @craig.topper wrote:

I can't remove the older processors from Host.cpp. Host.cpp also contains a newer processor called Goldmont that's not in libgcc yet. I think these files have slightly different goals.

In D28990#801641, @vchuravy wrote:

@echristo I noticed that you reverted r294702 shortly after it landed. What needs to be done to get it working?

In D35027#801294, @sfertile wrote:

Regarding the isIntS16Immediate change, the function is duplicated in PPCISelDAGToDAG.cpp as well. I suggest we leave it as is in this patch, and then in a separate patch common up the definitions and change it to use an int16_t argument. That keeps this patch focused on only doing a single thing, and puts the code cleanup in its own NFC patch.

This is how you should do it. Though I'm not sure it isn't just better to check for "processor >= power9", but *shrug* for now.

Hi Oliver!

I've committed this for you as:

I've gone ahead and committed something very similar thusly:
Committing to https://llvm.org/svn/llvm-project/llvm/trunk ...
M include/llvm/Support/TargetParser.h
M lib/Support/TargetParser.cpp
M lib/Target/ARM/ARMTargetMachine.cpp
M test/CodeGen/ARM/alloca.ll
M test/CodeGen/ARM/arm-abi-attr.ll
M test/CodeGen/ARM/ctor_order.ll
M test/CodeGen/ARM/ctors_dtors.ll
M test/CodeGen/ARM/ssp-data-layout.ll
M test/CodeGen/ARM/str_pre-2.ll
Committed r306768

LGTM.

Sorry, when I say "One inline comment otherwise LGTM" feel free to commit after fixing :)

Can you give me an example here of what you're trying to accomplish? I'm not sure I understand.

Reid is still the best reviewer for this.

This is OK once the dependent revision is approved.