LGTM, I think it's a nice simplification and was already discussed in D36899. Please check the formatting before committing.

Thanks for clarifying that! Sounds and looks good to me.

Please clarify the question about adding FeaturePostRAScheduler to some processors and maybe hold off a bit with committing, in case @rengolin or @

I think this change makes sense and will be convenient when we move ARM codegen to the MachineScheduler. I just have a quick question about adding FeaturePostRAScheduler to some processors.

In D36381#834280, @atrick wrote:

It sounds to me like you want a "MI-Sched Mode" convenience flag that does more than what it claims to do at face value, rather than needing to specify all the individual flags for SelectionDAG heuristic, enabling MI-Sched, coalescing heuristic. I'm not going to nit pick on the flag names because I don't have any stake it in. But I think you need to discuss this on llvm-dev and argue it out taking into consideration all of the goals:

• avoid confusions between the subtarget enableMachineScheduling() API and -mi-sched flag, which seems to have been a problem for you.
• provide "enable/disable" flags that don't do any magic beyond literally enabling or disabled the named pass, (that kind of magic been huge source of confusion in that past when it comes to triaging bugs and gathering performance data)
• unit tests should explicitly spell out precisely the passes/functionality that it depends on rather than relying on some umbrella "mode" that can change over time.

I think this changes makes sense and thanks for getting rid of the if (that's not needed any longer) and making the debug message clearer too! I guess adding a test for the case where SecondSU has dependencies that are scheduled between FirstSU and SecondSU is difficult, as in most fused instruction pairs the second instruction only depends on the first instruction?

Thanks for all the feedback! I'll update the patch soon.

Thanks for your feedback Eli. I've update the patch to only use incremental updates if the size of a BB is over a certain threshold. I've chosen 500 instructions for now, but a slightly higher one would probably be OK too.

LGTM. It's not used at the moment and should be removed.

Removed M-class triple check.

Ping. Any thoughts?

Looks like a straightforward NFC that makes the code clearer. LGTM, but maybe someone else should have a quick look too.

In D35569#836330, @echristo wrote:

In D35569#816289, @rengolin wrote:

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.

Well, clear or not is a bit subjective ;)

LGTM, this looks like a NFC to me and range-based for loops make the code easier to read. Please check the loop I added a comment to before committing.

I think this change should still go in (in combination with D35627) to get proper error messages in the attribute case. Please let me know if there are any more concerns.

In D36381#834116, @atrick wrote:

I don't understand the motivation here. The purpose of command line flags is to test functionality in isolation. If you want a test case to control both SelectionDag and MISched functionality, then you use two command line flags.

Remove function

Besides the outstanding comments, LGTM

@MatzeB are you happy with the change too? It appears you have to remove your 'This revision now requires changes to proceed.' setting before Phab will mark this patch as accepted.

Rebased

Thanks, I'm happy with the indentation and lib/Target/ARM/ARMISelDAGToDAG.cpp was a good spot.

LGTM, looks like a straight-forward improvement. I just have 2 nits. Please hold off with committing for a bit, so other people have time to voice concerns.

Using -start-before=livedebugvalues . I had to adjust the offset, but I don't think the exact offset is crucial for the test case. Maybe @aprantl can confirm that?

ping. @rengolin any more thoughts?

I'll commit D36151 after this one goes in, so we see if this change fixes the buildbot

Agreed, in hindsight the current solution is quite heavy-handed. 1 register

I just had a quick look and left a few comments. I'm just curious, what kind of testing did you do & did you run any benchmarks?

Yes, D36094 fixes the problem too! Here's what's going on in the test case, without any of the 2 patches:

I'm just curious, did you run any benchmarks with this change? I think it's quite clear that using the block frequencies is a good idea, but it would be awesome to know if/by how much it improves things :)

Updated to use LLVMContext::emitError instead of fatal error. Before committing D35569 needs to land, otherwise the second invocation of in the test will not produce errors.

In D34875#796985, @t.p.northover wrote:

I think that they do, because report_fatal_error() causes compilation to terminate so the subsequent tests don't generate a message which can be checked.

Ah yes. And that's also a really big indicator for why this should be handled in Clang. All you know from this is that at least one function, somewhere in your file (or codebase if doing LTO) has an aapcs_vfp attribute.

I get that silently doing SoftFP isn't great either so I'm not completely opposed to this change, I just think it's a pretty half-baked solution to the actual user-facing problem.

Thanks for having a look. Committed as is because some print functions are used in implementations of
raw_ostream &operator<<(raw_ostream &OS, const X &X), which in turn are used in DEBUG()

Added more checks to test case, thanks @mcrosier for having a look! :)

Renamed pseudo instructions, will commit soon

How about something like AESIMCrrTied?

ping

ping

ping

I think it shouldn't be to hard to fold the print() functions into the dump() functions in most cases. I could update the patch. The only benefit of having those print functions would be debugging, in the rare case someone wants to print to a different stream then stderr.

This patch also adds guards for a few functions not used by dump(), but only inside DEBUG(). I could split them off if that's preferred.