Ping? Tim, Renato?

Sorry for the long delay! Test case updated to make the long adds explicit, and not depend on constant hoisting.

Renato, perhaps you could take a look at this patch?

Hi Tim,

This needs a much better comment explaining why this is necessary

Added a comment explaining why this is necessary

Shouldn't we commit this now to fix ScheduleDAGRRList, and if/when similar problems are discovered with other schedulers that are used for ARM targets, to fix them then?

It's a def of CPSR if the S bit is set, otherwise it's a use of %noreg, or something like that, I think.

One, it prevents schedules which are perfectly legal for non-Thumb1 code.

No, it doesn't: for the test case that I'm adding, the Thumb2 output remains a sequence of (adds, eor, eor, adc).
This is because the code I'm adding checks not only the presence but also the actual value of the OptionalDef operand, and its value is %noreg when the instruction doesn't set CPSR.

Eli, do you mean that OptionalDefs should never be used because the scheduler (and perhaps other places) ignore them?

Can you make sure this works with inline asm? We had a trick in the to make it work (Linux stuff)

Updated to match the final revision of D30400

The minor tweaks

Lowering the negative-immediate operand as a DAGCombine instead

Adding tADCS/tSBCS pseudo-instructions does indeed let
simplify the custom selection code quite a bit, but
doesn't get rid of it entirely, as the negative-immediate
operand still needs a "recursive lowering" which cannot
be specified with ISel patterns. (This is similar to how
ISD::AND needs the custom lowering into a tBIC.)

When compiling for softfp targets, this new warning doesn't make sense: there are no VFP registers to save.
Jonathan, would you please conditionalize it to only affect hardfp targets?

Hybrid implementation

Updated test/CodeGen/ARM/smml.ll to check the handling of live CPSR (and cleaned up the CHECKs in general)

Copying the trick that the lowering for ISD::AND uses to create and lower a constant node

Select(RHS.getNode()) must be deferred until RHS has users; otherwise, if Select() converts RHS into a duplicate of an existing node, then the DAG automatically updates all uses of RHS to use the existing node instead, and deletes the RHS's own node.
If we call Select(RHS.getNode()) when RHS doesn't yet have any users, then nothing gets updated, RHS's node gets deleted, and we end up adding uses to a deleted node. Boom!

Added tests for SUBC with immediate

t2ADC should be predicable?

I'd think so too! As you see, long addition/subtraction is not the neatest part of the ARM backend :-)

Why not? "t2ADDSrr" is a pseudo-instruction, not an actual encoding.

Deleting the comment which is no longer relevant

Patch updated

clobbering CPSR when we don't need to is the least of the problems; what we have in ARM and Thumb2 is that ADD and ADDS are defined separately, the former producing one result (to match an ADD node), and the latter producing two (to match an ADDC node). In Thumb1, we cannot define them separately, so tADD MIs are defined with an OptionalDef for CPSR. The ISel patterns won't let me match an MI with one result value (and an OptionalDef) to an ISD node producing two results. Redefining tADD to always produce two results doesn't work either, because it's assumed, by many layers including AsmParser / AsmPrinter, to still have the OptionalDef for CPSR; and the InstrEmitter won't let me have CPSR as both an OptionalDef and an actual result in the same MI.
Handwave handwave, I cannot really prove that it cannot be done, but I mean I had tried, and I couldn't.

Are you sure we can't use the same codepath we currently use for Thumb2/ARM here?

Added LLVM_FALLTHROUGH

Patch updated

Thanks Eli!
Indeed the assertion was wrong; this also shows how insufficient our tests for long adds/subracts were.
Updating the patch to address both these points.

In D15746#381104, @echristo wrote:

I'm still uncertain we want to do this in the backend. I seem to recall you wanting to do this for a reason, but searching my email can't find it.

Ping again

In D15746#359578, @tyomitch wrote:

In D15746#348456, @tyomitch wrote:

In D15746#342099, @tyomitch wrote:

My patch would fix this inconsistency, so that "last one wins" would apply universally.

Ping?

Ping again

In D17636#364087, @rengolin wrote:

I'm not an expert in PatFrag, but this looks like a good move. @t.p.northover?

In D15746#348456, @tyomitch wrote:

In D15746#342099, @tyomitch wrote:

My patch would fix this inconsistency, so that "last one wins" would apply universally.

Ping?

In D15746#342099, @tyomitch wrote:

My patch would fix this inconsistency, so that "last one wins" would apply universally.

Thank you Eric for your detailed comments!

In D15746#326637, @tyomitch wrote:

Ping?

Ping?

Thank you Michael and Eric for your comments!
I've now committed the non-controversial changes as r256823.

Happy New Year everyone!

Ana, thank you for these new test cases.

Updating the patch to simplify ARMLoadStoreOpt::CombineMovBx

Updating the patch to handle the aforementioned additional cases of pop {lr} in test/CodeGen/Thumb/thumb-shrink-wrapping.ll

test/CodeGen/Thumb/thumb-shrink-wrapping.ll includes a few more cases when pop {lr} can still be generated; all of them are wrong, and need addressing.

In D15707#315285, @t.p.northover wrote:

Bother, I considered that but thought I'd investigated all the possible terminators when reviewing that patch and convinced myself they couldn't occur after a tPOP. Which one was it?

It's followed by an unconditional branch to a tBX_RET, and my patch only handled falling-through to a tBX_RET :-/

In D14759#296536, @tyomitch wrote:

Ping?

The two changes this patch depended on were both committed last week, so hopefully we can now proceed with this one, too.

Good point that if SDIVREM8_SEXT_HREG needs not be generated for an i64 result, then UDIVREM8_ZEXT_HREG doesn't need this either.

Ping?

Ping again

Quentin, thank you for the helpful suggestions!

Thanks for the review!

This ^ does not look correct for v4t, which doesn't support changing the thumb
bit via a pop into pc.

Would you mind leaving a FIXME in emitPopSpecialFixUp

The two changes this patch depended on were both committed last week, so hopefully we can now proceed with this one, too.

Updated as discussed

In D14945#296197, @MatzeB wrote:

I don't have a really good answer as there is no existing header/class that I would describe as containing shared utility functions for lowering
and adding a new header just for these 4 functions here seems overkill.

The best thing I can think of right now is adding something like "static bool isOneConstant(SDValue Value)" to the ConstantSDNode class.

In D14945#296046, @MatzeB wrote:

I agree that the code here could be shared.
However TargetLoweringBase provides an interface to customizing the lowering process for a specific target. I do not believe it is the right place for common lowering code.