[RFC,ARM] Fix PR43721 - combine multiple div and mod into a single divmod call.

Hi,

Though I don't claim to like this patch very much it seems to be the
(cough) "standard" way for backends to get the single divmodsi4
library call generated rather than writing a separate pass to detect
this . There has been some discussion about this with respect to the
audit trail for http://gcc.gnu.org/PR23726 but that talks about
writing a new pass to detect this or enhancing CSE to deal with
multiple SETs across presumably various rtl insns.

Ok to commit to trunk when stage1 opens and if tests show no regressions ?

Any other comments with respect to this would be more than welcome.


Ramana




2011-01-21  Ramana Radhakrishnan  <ramana.radhakrishnan@arm.com>

        PR target/43721
	gcc/
	* config/arm/arm.md (divmodsi4): New.
	(aeabi_divmodsi4_call): New.
	(aeabi_divsi3_call): New.
	(udivmodsi4): Likewise.
	(aeabi_udivmodsi4_call): New.
	(aeabi_udivsi3_call): New.

On 01/21/2011 04:37 PM, Ramana Radhakrishnan wrote:
> +(define_insn_and_split "divmodsi4"
> +  [(parallel [(set (match_operand:SI 0 "s_register_operand" "")
> +                   (div:SI (match_operand:SI 1 "s_register_operand" "")
> +                           (match_operand:SI 2 "s_register_operand" "")))
> +              (set (match_operand:SI 3 "s_register_operand" "")
> +                   (mod:SI (match_dup 1) (match_dup 2)))
> +              (clobber (reg:SI 0))
> +              (clobber (reg:SI 1))
> +              (clobber (reg:SI LR_REGNUM))])]

define_insn, unlike define_expand, has an implicit parallel.
You've got a redundant parallel in there.

> +  ""
> +  "#"
> +  "!(TARGET_THUMB2 && arm_arch_hwdiv)"

You have an unconditional insn, that is must-split, with a conditional
split?  That seems very wrong.  I suspect you want that condition moved
to the first "".

> +(define_insn "aeabi_divsi3_call"
> +  [(set (reg:SI 0) (div:SI (reg:SI 0) (reg:SI 1)))
> +   (clobber (reg:SI LR_REGNUM))]
> +  "!(TARGET_THUMB2 && arm_arch_hwdiv)"
> +  "bl\t__aeabi_idiv"
> +  [(set_attr "type" "call")
> +   (set_attr "conds" "clob")])

Does the eabi really say that no register except r0 is modified?
That doesn't seem to be the case, based on a quick glance over
the implementation in lib1funcs.asm.

If that's the case, your representation is very wrong.


r~

Hi Richard,


On Sun, Jan 23, 2011 at 10:49 PM, Richard Henderson <rth@redhat.com> wrote:
> On 01/21/2011 04:37 PM, Ramana Radhakrishnan wrote:
>> +(define_insn_and_split "divmodsi4"
>> +  [(parallel [(set (match_operand:SI 0 "s_register_operand" "")
>> +                   (div:SI (match_operand:SI 1 "s_register_operand" "")
>> +                           (match_operand:SI 2 "s_register_operand" "")))
>> +              (set (match_operand:SI 3 "s_register_operand" "")
>> +                   (mod:SI (match_dup 1) (match_dup 2)))
>> +              (clobber (reg:SI 0))
>> +              (clobber (reg:SI 1))
>> +              (clobber (reg:SI LR_REGNUM))])]
>
> define_insn, unlike define_expand, has an implicit parallel.
> You've got a redundant parallel in there.

Thanks for the review.

>
>> +  ""
>> +  "#"
>> +  "!(TARGET_THUMB2 && arm_arch_hwdiv)"
>
> You have an unconditional insn, that is must-split, with a conditional
> split?  That seems very wrong.  I suspect you want that condition moved
> to the first "".

Thanks for pointing that out.

>
>> +(define_insn "aeabi_divsi3_call"
>> +  [(set (reg:SI 0) (div:SI (reg:SI 0) (reg:SI 1)))
>> +   (clobber (reg:SI LR_REGNUM))]
>> +  "!(TARGET_THUMB2 && arm_arch_hwdiv)"
>> +  "bl\t__aeabi_idiv"
>> +  [(set_attr "type" "call")
>> +   (set_attr "conds" "clob")])
>
> Does the eabi really say that no register except r0 is modified?
> That doesn't seem to be the case, based on a quick glance over
> the implementation in lib1funcs.asm.

I've realized I should have moved the output function into a separate
function in arm.c that prints out the appropriate function name
depending on whether it is TARGET_AAPCS or not .

Yes I must admit I haven't covered all the caller saves that ideally
need to be marked as being clobbered rather than just those changed by
this implementation of _aeabi_idiv or _aeabi_idivmod since
the registers clobbered by _aeabi_div and _aeabi_divmod would vary
with each implementor of this function..

Sorry I must have mentioned this earlier but I was concerned more
about the approach in this mechanism where we fake a call insn through
the output template rather than through representing a call insn in
the RTL. Does anyone know if this is likely to cause problems
elsewhere ?


cheers
Ramana

On 01/24/2011 01:33 PM, Ramana Radhakrishnan wrote:
> Sorry I must have mentioned this earlier but I was concerned more
> about the approach in this mechanism where we fake a call insn through
> the output template rather than through representing a call insn in
> the RTL. Does anyone know if this is likely to cause problems
> elsewhere ?

As long as you're representing all of the register changes that are
possible, it should be ok.


r~