On Fri, Mar 01, 2019 at 03:41:33PM +0000, Wilco Dijkstra wrote: > >> This is overcomplicating something simple - adds/subs are completely > >> symmetrical on all Arm targets. So for addition you simply place adds > >> alternative first and then subs. For comparison/subtraction place the > > > > As I wrote, that is what I have done, > > That's not what your proposed patch does:
But the earlier version of that patch did, see the PR, in particular https://gcc.gnu.org/bugzilla/attachment.cgi?id=45840 > > - "TARGET_32BIT && INTVAL (operands[2]) == -INTVAL (operands[3])" > - "@ > - adds%?\\t%0, %1, %3 > - subs%?\\t%0, %1, #%n3" > + "TARGET_32BIT > + && (INTVAL (operands[2]) > + == trunc_int_for_mode (-INTVAL (operands[3]), SImode))" > +{ > + /* For 0 and INT_MIN it is essential that we use subs, as adds > + will result in different condition codes (like cmn rather than > + like cmp). For other immediates, we should choose whatever > + will have smaller encoding. */ > + if (operands[2] == const0_rtx > + || INTVAL (operands[2]) == -HOST_WIDE_INT_C (0x80000000) > + || which_alternative == 1) > + return "subs%?\\t%0, %1, #%n3"; > + else > + return "adds%?\\t%0, %1, %3"; > +} > > Adds/subs were in the incorrect order before and should simply be swapped > rather than replaced by complex C code (which would be unique just to this > pattern > when there are lot of similar patterns that do the right thing already). I don't see what's wrong on that, the code isn't really complex and actually explains what it does and why. If -1/1 is the only case where thumb2 cares, it will be trivial to add that. For operands[2] == const1_rtx we then want to use always adds and for operands[2] == constm1_rtx we want to use subs. Or swap the alternatives (then 0 and 0x80000000 will come out naturaly out of it) and just special case the operands[2] == const1_rtx case. That would then be for this hunk only following. Changing the "I" constraint or adding new constraints seems too risky or overkill. --- gcc/config/arm/arm.md.jj 2019-03-01 09:05:56.911097368 +0100 +++ gcc/config/arm/arm.md 2019-03-01 17:00:23.284076436 +0100 @@ -863,14 +863,24 @@ (define_insn "cmpsi2_addneg" [(set (reg:CC CC_REGNUM) (compare:CC (match_operand:SI 1 "s_register_operand" "r,r") - (match_operand:SI 2 "arm_addimm_operand" "L,I"))) + (match_operand:SI 2 "arm_addimm_operand" "I,L"))) (set (match_operand:SI 0 "s_register_operand" "=r,r") (plus:SI (match_dup 1) - (match_operand:SI 3 "arm_addimm_operand" "I,L")))] - "TARGET_32BIT && INTVAL (operands[2]) == -INTVAL (operands[3])" - "@ - adds%?\\t%0, %1, %3 - subs%?\\t%0, %1, #%n3" + (match_operand:SI 3 "arm_addimm_operand" "L,I")))] + "TARGET_32BIT + && (INTVAL (operands[2]) + == trunc_int_for_mode (-INTVAL (operands[3]), SImode))" +{ + /* For 0 and INT_MIN it is essential that we use subs, as adds + will result in different condition codes (like cmn rather than + like cmp). Both alternatives can match also for -1/1 with + TARGET_THUMB2, prefer instruction with #1 in that case as it + is shorter. */ + if (which_alternative == 0 && operands[1] != const1_rtx) + return "subs%?\\t%0, %1, #%n3"; + else + return "adds%?\\t%0, %1, %3"; +} [(set_attr "conds" "set") (set_attr "type" "alus_sreg")] ) Jakub
