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.

This is fine with me. I'll let Matthias do the final ack.

Can you send out a patch to make that comment on the base routine as well?

Looks OK to me.

Going to let Ben review this. I'd rather not pass the bool down and he might know a way to avoid that here by knowing the code more :)

Go ahead and submit the rename separately (if you don't have commit access send me a patch and I'll do it). I would prefer to try to minimize boolean arguments - in this case you're adding a few more including a true/false and a return value one. Feel free to refactor the code around it to require fewer booleans or have multiple functions with helper functions that end up being called.

There are two different perspectives here of assert vs unreachable vs other things.

Thanks!

Thanks for the fix!

One inline nit then LGTM.

Ah right. Thanks for looking.

One inline comment otherwise LGTM

Feel free to commit any patches that look like this to this area :)

Feel free to commit any patches that look like this to this area :)

LGTM.

SGTM. It still seems a bit overly complicated, but I don't think there's anything for it at the moment.