We should be able to optimize away the AND here using known bits; does that not happen on trunk?

These CHECK lines are not very good; try generating checks with utils/update_llc_test_checks.py. Also, please commit this first and rebase your patch on top of it; that makes it easy to see what happens to the generated code.

It doesn't because ARM doesn't call setBooleanContents. If there's a way to set that correctly, it would indeed be better. Do you think we could do that?

That tool does make the changes to the testfiles more obvious, but it also generates overly-specific testcases that seem more brittle. I'll see if I can make a nice middle-ground and update it in a bit.

Really? Wow, that's a big oversight. We should fix that.

Do you know the correct way to set it? I tried, but a few tests failed, and I don't know the architecture well enough to know if that was my fault or of the tests just needed to be tweaked.

If we do make that change, then I'll be able to simplify this code, of course.

Okay, I've uploaded both the old and new versions of the test here. Thanks for that tool, by the way; it's pretty convenient.

It's likely the tests just need to be tweaked; a lot of them are pretty sensitive to the exact compiler output.

So which argument to setBooleanContents is correct for ARM? Is it ZeroOrOne?

ZeroOrOne is probably best; among other things, it matches the AAPCS calling convention rules for bool.

Okay, thanks. I'll look into a patch that simply adds that and then rebase this commit on top of it, which will allow me to remove the AND-checking logic here.

This looks weird; we're generating a cmp, then a sub with exactly the same operands?

Well, as you can see in diff 2, we're currently doing that with a lot of other instructions. This removes most of those other instructions. I have another patch I was going to send after this one that fixes this.

Specifically, ARMBaseInstrInfo::optimizeCompareInstr tries to remove the cmp, but it runs right after the MachineSink pass, which sinks the sub into the cont basic block, which stops the optimization from working.

Still the wrong type... you need to call getVTList to get the right type for an ADDC.

Also, are you sure you don't want an ARMISD::ADDC, rather than an ISD::ADDC?

Is this assert actually guaranteed somehow? I mean, it should be possible to transform any relevant condition to an equivalent SETEQ or SETNE, but I don't see any code to actually ensure this.

I would probably write "if ((CC == ISD::SETNE) != isOneConstant(RHS))".

Good point. I actually copied that part of the code over from the AArch64 backend. I would guess that the assumption is that you should only be checking if the overflow bit is set or unset, hence EQ or NE, but I could imagine something transforming those into other operations. I changed it to be part of the condition to avoid the problem. Do you think it's worth doing something similar in the AArch64 backend?

Yes.

Okay, I'll work on submitting a patch there (it should be NFC).

Apologies if this question looks a bit clueless about all the transformation, but why you need to reverse the condition code here?

No, it's a good question, and I'm a bit confused about this myself.

The getARMXALUOOp function seems to return the condition for whether there is not an overflow. It doesn't seem to be documented anywhere, but for example, in its SADDO case, it uses the VC flag, which is the "no overflow" case.

Assuming that's right, then since these branches are branching when an overflow occurs, I need to invert the condition.

Does that logic seem right?

We could return std::tuple<SDValue, SDValue, SDValue> here I think, but better we leave this for a later change.

It is certainly confusing.

It returns three things: the intended arithmetic code (in Value), the comparison of the overflow (in OverflowCmp) and a condition code related to OverflowCmp (in ARMcc). For unsigned addition (UADDO) it does the following

Value: add z, x, y
OverflowCmp: cmp z, x
ARMcc: HS

The comparison will compute z - x and update the flags. Using the HS condition code means z >= x and this is exactly *no overflow* (because z ← x + y).

Similarly when doing a signed addition (SADDO)

Value: add z, x, y
OverflowCmp: cmp z, x
ARMcc: VC

This case is less obvious but if there is no overflow in this case there must not be overflow in the comparison (VC): if there were no overflow in the add, then cmp because it does z - x to update the flags would compute y which does not overflow.

Similarly for USUBO and SSUBO but here the comparison is cmp x, y (instead of cmp z, x) which is (suprisingly) more intuitive because unsigned x - y will not overflow if x >= y and trivially for the signed case as not overflowing is exactly VC.

So after this wall of text, I believe your logic is sensible here.

Given that getARMXALUOOp is now going to be used in three places after your change, would it be possible to add a comment to that function like the one below (or an improved wording if you come up with a better one!)

// This function returns three things: the arithmetic computation itself
// (Value), a comparison (OverflowCmp) and a condition code (ARMcc).  The
// comparison and the condition code define the case in which the arithmetic
// computation *does not* overflow.
std::pair<SDValue, SDValue>
ARMTargetLowering::getARMXALUOOp(SDValue Op, SelectionDAG &DAG,
                                 SDValue &ARMcc) const {
...

This comment now looks odd because I had to revert D35192 (still working on it, though).

What was the reason to use a target specific node here? Did you want to save some round trips lowering this or you needed it for better codegen?

Yes, that does seem cleaner to me. I can make a follow-up patch after this lands.

Thanks for the long double-checking! That comment looks good to me, so I'll add it.

I used the target-specific node here because efriedma suggested it, but using the generic ISD::ADDC does seem to work (it passes all the tests). Should I change it? I don't know much about the difference between them.

It is interesting that this still works after you reverted your patch.

My understanding is that several generic nodes (ISD::*) have to be lowered at some point to Arm-specific ones (ARMISD::*). ISD::ADD and ISD::SUB are among those nodes. Using Arm-specific nodes earlier might bring finer control but may miss some of the generic combiners done on generic nodes.

But it is true that when this function is used, the generic node will be found among specific ones (like ARMISD::CMP) so generic combiners are unlikely to kick in here, which might be a reason to use the specific one directly and not bothering using the generic one (which I presume will require an extra iteration replacing it with a specific one).

Changing just this case will make this function look a bit odd. If Eli's comment was in the line of "let's make this function only use ARMISD" (instead of ISD::ADD and ISD::SUB) we can make this change in a later patch and leave this function untouched (except for the comment).

@efriedma am I missing something?

Please don't use ISD::ADDC here; now that we're using ADDCARRY, we shouldn't produce that node at all on ARM. (LowerADDC_ADDE_SUBC_SUBE should have been removed as part of D35192. ARMISD::ADDC is a different node which is still useful.)

Thanks @efriedma. Now I see that I should have removed LowerADDC_ADDE_SUBC_SUBE in D35192.

That said I fail to understand why we want to update this function in this patch. @jgalenson does keep using ISD:ADD (not to confuse it with ISD::ADDC) impact the code generation?

I mean, I understand we can use ISD::ADDCARRY here but maybe we want to do this in a later patch?

Yes, with D35192 landed using ISD::ADD produces worse code. It's possible there's a different way to fix that, but doing this works as well.