[PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[Prathamesh Kulkarni]

Hi,
I would like to ping the following patch:
https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01015.html

While implementing divmod transform:
https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html

I ran into an  issue with optab_libfunc().
It appears optab_libfunc (sdivmod_optab, mode) returns
bogus libfunc for unsupported modes, for instance
on x86_64, optab_libfunc (sdivmod_optab, DImode) returns
a libfunc with name "__divmoddi4", even though such a libfunc
does not exist in libgcc. This happens because in optabs.def
the libfunc entry for sdivmod_optab has gen_int_libfunc,
and call to optab_libfunc (sdivmo_optab, DImode) lazily
creates a bogus libfunc "__divmoddi4" by calling gen_int_libfunc().

To work around this issue I set libfunc entry for sdivmod_optab to NULL
and verified that optab_libfunc (sdivmod_optab, DImode) returns NULL_RTX
instead of a bogus libfunc if it's not overriden by the target.

Bootstrapped and tested on ppc64le-linux-gnu, x86_64-linux-gnu.
Cross tested on arm*-*-*, aarch64*-*-*.
OK for trunk ?

Thanks,
Prathamesh

Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[Richard Sandiford]

Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> Hi,
> I would like to ping the following patch:
> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01015.html
>
> While implementing divmod transform:
> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>
> I ran into an  issue with optab_libfunc().
> It appears optab_libfunc (sdivmod_optab, mode) returns
> bogus libfunc for unsupported modes, for instance
> on x86_64, optab_libfunc (sdivmod_optab, DImode) returns
> a libfunc with name "__divmoddi4", even though such a libfunc
> does not exist in libgcc. This happens because in optabs.def
> the libfunc entry for sdivmod_optab has gen_int_libfunc,
> and call to optab_libfunc (sdivmo_optab, DImode) lazily
> creates a bogus libfunc "__divmoddi4" by calling gen_int_libfunc().
>
> To work around this issue I set libfunc entry for sdivmod_optab to NULL
> and verified that optab_libfunc (sdivmod_optab, DImode) returns NULL_RTX
> instead of a bogus libfunc if it's not overriden by the target.
>
> Bootstrapped and tested on ppc64le-linux-gnu, x86_64-linux-gnu.
> Cross tested on arm*-*-*, aarch64*-*-*.
> OK for trunk ?

I'm not a maintainer for this area, but:

Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[Richard Sandiford]

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> Hi,
>> I would like to ping the following patch:
>> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01015.html
>>
>> While implementing divmod transform:
>> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>
>> I ran into an  issue with optab_libfunc().
>> It appears optab_libfunc (sdivmod_optab, mode) returns
>> bogus libfunc for unsupported modes, for instance
>> on x86_64, optab_libfunc (sdivmod_optab, DImode) returns
>> a libfunc with name "__divmoddi4", even though such a libfunc
>> does not exist in libgcc. This happens because in optabs.def
>> the libfunc entry for sdivmod_optab has gen_int_libfunc,
>> and call to optab_libfunc (sdivmo_optab, DImode) lazily
>> creates a bogus libfunc "__divmoddi4" by calling gen_int_libfunc().
>>
>> To work around this issue I set libfunc entry for sdivmod_optab to NULL
>> and verified that optab_libfunc (sdivmod_optab, DImode) returns NULL_RTX
>> instead of a bogus libfunc if it's not overriden by the target.
>>
>> Bootstrapped and tested on ppc64le-linux-gnu, x86_64-linux-gnu.
>> Cross tested on arm*-*-*, aarch64*-*-*.
>> OK for trunk ?
>
> I'm not a maintainer for this area, but:

...in https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
you said that c6x follows the return-by-pointer convention.
I'm no c6x expert, but from a quick look, I think its divrem
function returns a div/mod pair in A4/A5, which matches the
ARM convention of returning both results by value.

Does anyone know if the optab function registered by the SPU
backend is ever called directly?

You mention that this is latent as far as expand_twoval_binop_libfunc
is concerned.  AIUI expand_twoval_binop_libfunc implements the ARM/c6x
convention and expects the two values to be returned as a pair.
It then extracts one half of the pair and discards the other.
So my worry is that we're leaving the udivmod entry intact even though
the standard __udivmoddi4 doesn't do what expand_twoval_binop_libfunc
expects.

Would it make sense to set both entries to null and treat __udivmoddi4
as a non-optab function?  ARM and c6x could then continue to register
their current optab functions and a non-null optab function would
indicate a return value pair.

Sorry if this has already been covered.

Thanks,
Richard

Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[Prathamesh Kulkarni]

On 15 September 2016 at 04:21, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>> Hi,
>>> I would like to ping the following patch:
>>> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01015.html
>>>
>>> While implementing divmod transform:
>>> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>>
>>> I ran into an  issue with optab_libfunc().
>>> It appears optab_libfunc (sdivmod_optab, mode) returns
>>> bogus libfunc for unsupported modes, for instance
>>> on x86_64, optab_libfunc (sdivmod_optab, DImode) returns
>>> a libfunc with name "__divmoddi4", even though such a libfunc
>>> does not exist in libgcc. This happens because in optabs.def
>>> the libfunc entry for sdivmod_optab has gen_int_libfunc,
>>> and call to optab_libfunc (sdivmo_optab, DImode) lazily
>>> creates a bogus libfunc "__divmoddi4" by calling gen_int_libfunc().
>>>
>>> To work around this issue I set libfunc entry for sdivmod_optab to NULL
>>> and verified that optab_libfunc (sdivmod_optab, DImode) returns NULL_RTX
>>> instead of a bogus libfunc if it's not overriden by the target.
>>>
>>> Bootstrapped and tested on ppc64le-linux-gnu, x86_64-linux-gnu.
>>> Cross tested on arm*-*-*, aarch64*-*-*.
>>> OK for trunk ?
>>
>> I'm not a maintainer for this area, but:
>
> ...in https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
> you said that c6x follows the return-by-pointer convention.
> I'm no c6x expert, but from a quick look, I think its divrem
> function returns a div/mod pair in A4/A5, which matches the
> ARM convention of returning both results by value.
>
> Does anyone know if the optab function registered by the SPU
> backend is ever called directly?
>
> You mention that this is latent as far as expand_twoval_binop_libfunc
> is concerned.  AIUI expand_twoval_binop_libfunc implements the ARM/c6x
> convention and expects the two values to be returned as a pair.
> It then extracts one half of the pair and discards the other.
> So my worry is that we're leaving the udivmod entry intact even though
> the standard __udivmoddi4 doesn't do what expand_twoval_binop_libfunc
> expects.
>
> Would it make sense to set both entries to null and treat __udivmoddi4
> as a non-optab function?  ARM and c6x could then continue to register
> their current optab functions and a non-null optab function would
> indicate a return value pair.
AFAIU, there are only three targets (c6x, spu, arm) that override
optab_libfunc for udivmod_optab for following modes:
./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, SImode, "__c6xabi_divremu");
./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, DImode, "__c6xabi_divremull");
./arm/arm.c:  set_optab_libfunc (udivmod_optab, DImode, "__aeabi_uldivmod");
./arm/arm.c:  set_optab_libfunc (udivmod_optab, SImode, "__aeabi_uidivmod");
./spu/spu.c:  set_optab_libfunc (udivmod_optab, DImode, "__udivmoddi4");
./spu/spu.c:  set_optab_libfunc (udivmod_optab, TImode, "__udivmodti4");

Out of these only the arm, and c6x have target-specific divmod libfuncs which
return <div, mod> pair, while spu merely makes it point to the
standard functions.

So we could set libfunc entry for udivmod_optab to NULL, thus dropping
support for generic
divmod functions (__udivmoddi4, __udivmodti4). For targets that
require standard divmod libfuncs like __udivmoddi4,
they could explicitly override  optab_libfunc and set it to
__udivmoddi4, just as spu does.
However this implies that non-null optab function doesn't necessarily
follow arm/c6x convention.
(i686-gcc for instance generates call to libgcc routines
__udivdi3/__umoddi3 for DImode division/mod operations
and could profit from divmod transform by calling __udivmoddi4).

However then the issue with expand_twoval_optab_libfunc() still remains, and
I am not sure what to do about that.
As a temporary hack, we could punt if the divmod function's name is
"__udivmoddi4":

--- a/gcc/optabs.c
+++ b/gcc/optabs.c
@@ -2083,7 +2083,7 @@ expand_twoval_binop_libfunc (optab binoptab, rtx
op0, rtx op1,

   mode = GET_MODE (op0);
   libfunc = optab_libfunc (binoptab, mode);
-  if (!libfunc)
+  if (!libfunc || !strcmp (XSTR (libfunc, 0), "__udivmoddi4"))
     return false;

which is admittedly quite ugly :/

Thanks,
Prathamesh
>
> Sorry if this has already been covered.
>
> Thanks,
> Richard

Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[Richard Sandiford]

Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
> On 15 September 2016 at 04:21, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>>> Hi,
>>>> I would like to ping the following patch:
>>>> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01015.html
>>>>
>>>> While implementing divmod transform:
>>>> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>>>
>>>> I ran into an  issue with optab_libfunc().
>>>> It appears optab_libfunc (sdivmod_optab, mode) returns
>>>> bogus libfunc for unsupported modes, for instance
>>>> on x86_64, optab_libfunc (sdivmod_optab, DImode) returns
>>>> a libfunc with name "__divmoddi4", even though such a libfunc
>>>> does not exist in libgcc. This happens because in optabs.def
>>>> the libfunc entry for sdivmod_optab has gen_int_libfunc,
>>>> and call to optab_libfunc (sdivmo_optab, DImode) lazily
>>>> creates a bogus libfunc "__divmoddi4" by calling gen_int_libfunc().
>>>>
>>>> To work around this issue I set libfunc entry for sdivmod_optab to NULL
>>>> and verified that optab_libfunc (sdivmod_optab, DImode) returns NULL_RTX
>>>> instead of a bogus libfunc if it's not overriden by the target.
>>>>
>>>> Bootstrapped and tested on ppc64le-linux-gnu, x86_64-linux-gnu.
>>>> Cross tested on arm*-*-*, aarch64*-*-*.
>>>> OK for trunk ?
>>>
>>> I'm not a maintainer for this area, but:
>>
>> ...in https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>> you said that c6x follows the return-by-pointer convention.
>> I'm no c6x expert, but from a quick look, I think its divrem
>> function returns a div/mod pair in A4/A5, which matches the
>> ARM convention of returning both results by value.
>>
>> Does anyone know if the optab function registered by the SPU
>> backend is ever called directly?
>>
>> You mention that this is latent as far as expand_twoval_binop_libfunc
>> is concerned.  AIUI expand_twoval_binop_libfunc implements the ARM/c6x
>> convention and expects the two values to be returned as a pair.
>> It then extracts one half of the pair and discards the other.
>> So my worry is that we're leaving the udivmod entry intact even though
>> the standard __udivmoddi4 doesn't do what expand_twoval_binop_libfunc
>> expects.
>>
>> Would it make sense to set both entries to null and treat __udivmoddi4
>> as a non-optab function?  ARM and c6x could then continue to register
>> their current optab functions and a non-null optab function would
>> indicate a return value pair.
> AFAIU, there are only three targets (c6x, spu, arm) that override
> optab_libfunc for udivmod_optab for following modes:
> ./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, SImode, "__c6xabi_divremu");
> ./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, DImode, "__c6xabi_divremull");
> ./arm/arm.c:  set_optab_libfunc (udivmod_optab, DImode, "__aeabi_uldivmod");
> ./arm/arm.c:  set_optab_libfunc (udivmod_optab, SImode, "__aeabi_uidivmod");
> ./spu/spu.c:  set_optab_libfunc (udivmod_optab, DImode, "__udivmoddi4");
> ./spu/spu.c:  set_optab_libfunc (udivmod_optab, TImode, "__udivmodti4");
>
> Out of these only the arm, and c6x have target-specific divmod libfuncs which
> return <div, mod> pair, while spu merely makes it point to the
> standard functions.
>
> So we could set libfunc entry for udivmod_optab to NULL, thus dropping
> support for generic
> divmod functions (__udivmoddi4, __udivmodti4). For targets that
> require standard divmod libfuncs like __udivmoddi4,
> they could explicitly override  optab_libfunc and set it to
> __udivmoddi4, just as spu does.
>
> However this implies that non-null optab function doesn't necessarily
> follow arm/c6x convention.
> (i686-gcc for instance generates call to libgcc routines
> __udivdi3/__umoddi3 for DImode division/mod operations
> and could profit from divmod transform by calling __udivmoddi4).

What I meant was that we shouldn't treat udivmoddi4 as an optab function
at all, but handle it with some on-the-side mechanism.  That seems like
quite a natural fit if we handle the fused div/mod operation as an
internal function during gimple.

I think the current SPU code is wrong, but it looks like a latent bug.
(Like I say, does the udivmodti4 function that it registers ever
actually get used?  It seems unlikely.)

In that scenario no other targets should do what SPU does.

Thanks,
Richard

Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 5044 bytes --]

On 15 September 2016 at 16:31, Richard Sandiford
<richard.sandiford@arm.com> wrote:
> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>> On 15 September 2016 at 04:21, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>>>> Hi,
>>>>> I would like to ping the following patch:
>>>>> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01015.html
>>>>>
>>>>> While implementing divmod transform:
>>>>> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>>>>
>>>>> I ran into an  issue with optab_libfunc().
>>>>> It appears optab_libfunc (sdivmod_optab, mode) returns
>>>>> bogus libfunc for unsupported modes, for instance
>>>>> on x86_64, optab_libfunc (sdivmod_optab, DImode) returns
>>>>> a libfunc with name "__divmoddi4", even though such a libfunc
>>>>> does not exist in libgcc. This happens because in optabs.def
>>>>> the libfunc entry for sdivmod_optab has gen_int_libfunc,
>>>>> and call to optab_libfunc (sdivmo_optab, DImode) lazily
>>>>> creates a bogus libfunc "__divmoddi4" by calling gen_int_libfunc().
>>>>>
>>>>> To work around this issue I set libfunc entry for sdivmod_optab to NULL
>>>>> and verified that optab_libfunc (sdivmod_optab, DImode) returns NULL_RTX
>>>>> instead of a bogus libfunc if it's not overriden by the target.
>>>>>
>>>>> Bootstrapped and tested on ppc64le-linux-gnu, x86_64-linux-gnu.
>>>>> Cross tested on arm*-*-*, aarch64*-*-*.
>>>>> OK for trunk ?
>>>>
>>>> I'm not a maintainer for this area, but:
>>>
>>> ...in https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>> you said that c6x follows the return-by-pointer convention.
>>> I'm no c6x expert, but from a quick look, I think its divrem
>>> function returns a div/mod pair in A4/A5, which matches the
>>> ARM convention of returning both results by value.
>>>
>>> Does anyone know if the optab function registered by the SPU
>>> backend is ever called directly?
>>>
>>> You mention that this is latent as far as expand_twoval_binop_libfunc
>>> is concerned.  AIUI expand_twoval_binop_libfunc implements the ARM/c6x
>>> convention and expects the two values to be returned as a pair.
>>> It then extracts one half of the pair and discards the other.
>>> So my worry is that we're leaving the udivmod entry intact even though
>>> the standard __udivmoddi4 doesn't do what expand_twoval_binop_libfunc
>>> expects.
>>>
>>> Would it make sense to set both entries to null and treat __udivmoddi4
>>> as a non-optab function?  ARM and c6x could then continue to register
>>> their current optab functions and a non-null optab function would
>>> indicate a return value pair.
>> AFAIU, there are only three targets (c6x, spu, arm) that override
>> optab_libfunc for udivmod_optab for following modes:
>> ./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, SImode, "__c6xabi_divremu");
>> ./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, DImode, "__c6xabi_divremull");
>> ./arm/arm.c:  set_optab_libfunc (udivmod_optab, DImode, "__aeabi_uldivmod");
>> ./arm/arm.c:  set_optab_libfunc (udivmod_optab, SImode, "__aeabi_uidivmod");
>> ./spu/spu.c:  set_optab_libfunc (udivmod_optab, DImode, "__udivmoddi4");
>> ./spu/spu.c:  set_optab_libfunc (udivmod_optab, TImode, "__udivmodti4");
>>
>> Out of these only the arm, and c6x have target-specific divmod libfuncs which
>> return <div, mod> pair, while spu merely makes it point to the
>> standard functions.
>>
>> So we could set libfunc entry for udivmod_optab to NULL, thus dropping
>> support for generic
>> divmod functions (__udivmoddi4, __udivmodti4). For targets that
>> require standard divmod libfuncs like __udivmoddi4,
>> they could explicitly override  optab_libfunc and set it to
>> __udivmoddi4, just as spu does.
>>
>> However this implies that non-null optab function doesn't necessarily
>> follow arm/c6x convention.
>> (i686-gcc for instance generates call to libgcc routines
>> __udivdi3/__umoddi3 for DImode division/mod operations
>> and could profit from divmod transform by calling __udivmoddi4).
>
> What I meant was that we shouldn't treat udivmoddi4 as an optab function
> at all, but handle it with some on-the-side mechanism.  That seems like
> quite a natural fit if we handle the fused div/mod operation as an
> internal function during gimple.
Ah right, thanks for pointing out. So if optab function for [us]divmod_optab
is defined, then it must follow the arm/c6x convention ?
>
> I think the current SPU code is wrong, but it looks like a latent bug.
> (Like I say, does the udivmodti4 function that it registers ever
> actually get used?  It seems unlikely.)
>
> In that scenario no other targets should do what SPU does.
I am testing the following patch which sets libfunc entries for both
sdivmod_optab, udivmod_optab to NULL.
This won't change the current (broken) behavior for SPU port since it
explicitly overrides optab_libfunc for udivmod_optab
and sets it to __udivmoddi4.

Thanks,
Prathamesh
>
> Thanks,
> Richard

[-- Attachment #2: foo.diff --]
[-- Type: text/plain, Size: 940 bytes --]

diff --git a/gcc/optabs.def b/gcc/optabs.def
index 8875e30..b37ac2e 100644
--- a/gcc/optabs.def
+++ b/gcc/optabs.def
@@ -116,8 +116,8 @@ OPTAB_NL(ssdiv_optab, "ssdiv$Q$a3", SS_DIV, "ssdiv", '3', gen_signed_fixed_libfu
 OPTAB_NL(udiv_optab, "udiv$I$a3", UDIV, "udiv", '3', gen_int_unsigned_fixed_libfunc)
 OPTAB_NX(udiv_optab, "udiv$Q$a3")
 OPTAB_NL(usdiv_optab, "usdiv$Q$a3", US_DIV, "usdiv", '3', gen_unsigned_fixed_libfunc)
-OPTAB_NL(sdivmod_optab, "divmod$a4", UNKNOWN, "divmod", '4', gen_int_libfunc)
-OPTAB_NL(udivmod_optab, "udivmod$a4", UNKNOWN, "udivmod", '4', gen_int_libfunc)
+OPTAB_NL(sdivmod_optab, "divmod$a4", UNKNOWN, "divmod", '4', NULL) 
+OPTAB_NL(udivmod_optab, "udivmod$a4", UNKNOWN, "udivmod", '4', NULL) 
 OPTAB_NL(smod_optab, "mod$a3", MOD, "mod", '3', gen_int_libfunc)
 OPTAB_NL(umod_optab, "umod$a3", UMOD, "umod", '3', gen_int_libfunc)
 OPTAB_NL(ftrunc_optab, "ftrunc$F$a2", UNKNOWN, "ftrunc", '2', gen_fp_libfunc)

Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[Richard Biener]

On Thu, Sep 15, 2016 at 1:21 PM, Prathamesh Kulkarni
<prathamesh.kulkarni@linaro.org> wrote:
> On 15 September 2016 at 16:31, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>> On 15 September 2016 at 04:21, Richard Sandiford
>>> <rdsandiford@googlemail.com> wrote:
>>>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>>>>> Hi,
>>>>>> I would like to ping the following patch:
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01015.html
>>>>>>
>>>>>> While implementing divmod transform:
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>>>>>
>>>>>> I ran into an  issue with optab_libfunc().
>>>>>> It appears optab_libfunc (sdivmod_optab, mode) returns
>>>>>> bogus libfunc for unsupported modes, for instance
>>>>>> on x86_64, optab_libfunc (sdivmod_optab, DImode) returns
>>>>>> a libfunc with name "__divmoddi4", even though such a libfunc
>>>>>> does not exist in libgcc. This happens because in optabs.def
>>>>>> the libfunc entry for sdivmod_optab has gen_int_libfunc,
>>>>>> and call to optab_libfunc (sdivmo_optab, DImode) lazily
>>>>>> creates a bogus libfunc "__divmoddi4" by calling gen_int_libfunc().
>>>>>>
>>>>>> To work around this issue I set libfunc entry for sdivmod_optab to NULL
>>>>>> and verified that optab_libfunc (sdivmod_optab, DImode) returns NULL_RTX
>>>>>> instead of a bogus libfunc if it's not overriden by the target.
>>>>>>
>>>>>> Bootstrapped and tested on ppc64le-linux-gnu, x86_64-linux-gnu.
>>>>>> Cross tested on arm*-*-*, aarch64*-*-*.
>>>>>> OK for trunk ?
>>>>>
>>>>> I'm not a maintainer for this area, but:
>>>>
>>>> ...in https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>>> you said that c6x follows the return-by-pointer convention.
>>>> I'm no c6x expert, but from a quick look, I think its divrem
>>>> function returns a div/mod pair in A4/A5, which matches the
>>>> ARM convention of returning both results by value.
>>>>
>>>> Does anyone know if the optab function registered by the SPU
>>>> backend is ever called directly?
>>>>
>>>> You mention that this is latent as far as expand_twoval_binop_libfunc
>>>> is concerned.  AIUI expand_twoval_binop_libfunc implements the ARM/c6x
>>>> convention and expects the two values to be returned as a pair.
>>>> It then extracts one half of the pair and discards the other.
>>>> So my worry is that we're leaving the udivmod entry intact even though
>>>> the standard __udivmoddi4 doesn't do what expand_twoval_binop_libfunc
>>>> expects.
>>>>
>>>> Would it make sense to set both entries to null and treat __udivmoddi4
>>>> as a non-optab function?  ARM and c6x could then continue to register
>>>> their current optab functions and a non-null optab function would
>>>> indicate a return value pair.
>>> AFAIU, there are only three targets (c6x, spu, arm) that override
>>> optab_libfunc for udivmod_optab for following modes:
>>> ./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, SImode, "__c6xabi_divremu");
>>> ./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, DImode, "__c6xabi_divremull");
>>> ./arm/arm.c:  set_optab_libfunc (udivmod_optab, DImode, "__aeabi_uldivmod");
>>> ./arm/arm.c:  set_optab_libfunc (udivmod_optab, SImode, "__aeabi_uidivmod");
>>> ./spu/spu.c:  set_optab_libfunc (udivmod_optab, DImode, "__udivmoddi4");
>>> ./spu/spu.c:  set_optab_libfunc (udivmod_optab, TImode, "__udivmodti4");
>>>
>>> Out of these only the arm, and c6x have target-specific divmod libfuncs which
>>> return <div, mod> pair, while spu merely makes it point to the
>>> standard functions.
>>>
>>> So we could set libfunc entry for udivmod_optab to NULL, thus dropping
>>> support for generic
>>> divmod functions (__udivmoddi4, __udivmodti4). For targets that
>>> require standard divmod libfuncs like __udivmoddi4,
>>> they could explicitly override  optab_libfunc and set it to
>>> __udivmoddi4, just as spu does.
>>>
>>> However this implies that non-null optab function doesn't necessarily
>>> follow arm/c6x convention.
>>> (i686-gcc for instance generates call to libgcc routines
>>> __udivdi3/__umoddi3 for DImode division/mod operations
>>> and could profit from divmod transform by calling __udivmoddi4).
>>
>> What I meant was that we shouldn't treat udivmoddi4 as an optab function
>> at all, but handle it with some on-the-side mechanism.  That seems like
>> quite a natural fit if we handle the fused div/mod operation as an
>> internal function during gimple.
> Ah right, thanks for pointing out. So if optab function for [us]divmod_optab
> is defined, then it must follow the arm/c6x convention ?
>>
>> I think the current SPU code is wrong, but it looks like a latent bug.
>> (Like I say, does the udivmodti4 function that it registers ever
>> actually get used?  It seems unlikely.)
>>
>> In that scenario no other targets should do what SPU does.
> I am testing the following patch which sets libfunc entries for both
> sdivmod_optab, udivmod_optab to NULL.
> This won't change the current (broken) behavior for SPU port since it
> explicitly overrides optab_libfunc for udivmod_optab
> and sets it to __udivmoddi4.

Just

-OPTAB_NL(sdivmod_optab, "divmod$a4", UNKNOWN, "divmod", '4', gen_int_libfunc)
-OPTAB_NL(udivmod_optab, "udivmod$a4", UNKNOWN, "udivmod", '4', gen_int_libfunc)
+OPTAB_NC(sdivmod_optab, "divmod$a4", UNKNOWN)
+OPTAB_NC(udivmod_optab, "udivmod$a4", UNKNOWN)

If I remember correctly.

Richard.


> Thanks,
> Prathamesh
>>
>> Thanks,
>> Richard

Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def

[Prathamesh Kulkarni]

[-- Attachment #1: Type: text/plain, Size: 7492 bytes --]

On 15 September 2016 at 17:53, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Thu, Sep 15, 2016 at 1:21 PM, Prathamesh Kulkarni
> <prathamesh.kulkarni@linaro.org> wrote:
>> On 15 September 2016 at 16:31, Richard Sandiford
>> <richard.sandiford@arm.com> wrote:
>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>>> On 15 September 2016 at 04:21, Richard Sandiford
>>>> <rdsandiford@googlemail.com> wrote:
>>>>> Richard Sandiford <rdsandiford@googlemail.com> writes:
>>>>>> Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org> writes:
>>>>>>> Hi,
>>>>>>> I would like to ping the following patch:
>>>>>>> https://gcc.gnu.org/ml/gcc-patches/2016-08/msg01015.html
>>>>>>>
>>>>>>> While implementing divmod transform:
>>>>>>> https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>>>>>>
>>>>>>> I ran into an  issue with optab_libfunc().
>>>>>>> It appears optab_libfunc (sdivmod_optab, mode) returns
>>>>>>> bogus libfunc for unsupported modes, for instance
>>>>>>> on x86_64, optab_libfunc (sdivmod_optab, DImode) returns
>>>>>>> a libfunc with name "__divmoddi4", even though such a libfunc
>>>>>>> does not exist in libgcc. This happens because in optabs.def
>>>>>>> the libfunc entry for sdivmod_optab has gen_int_libfunc,
>>>>>>> and call to optab_libfunc (sdivmo_optab, DImode) lazily
>>>>>>> creates a bogus libfunc "__divmoddi4" by calling gen_int_libfunc().
>>>>>>>
>>>>>>> To work around this issue I set libfunc entry for sdivmod_optab to NULL
>>>>>>> and verified that optab_libfunc (sdivmod_optab, DImode) returns NULL_RTX
>>>>>>> instead of a bogus libfunc if it's not overriden by the target.
>>>>>>>
>>>>>>> Bootstrapped and tested on ppc64le-linux-gnu, x86_64-linux-gnu.
>>>>>>> Cross tested on arm*-*-*, aarch64*-*-*.
>>>>>>> OK for trunk ?
>>>>>>
>>>>>> I'm not a maintainer for this area, but:
>>>>>
>>>>> ...in https://gcc.gnu.org/ml/gcc-patches/2016-05/msg01757.html
>>>>> you said that c6x follows the return-by-pointer convention.
>>>>> I'm no c6x expert, but from a quick look, I think its divrem
>>>>> function returns a div/mod pair in A4/A5, which matches the
>>>>> ARM convention of returning both results by value.
>>>>>
>>>>> Does anyone know if the optab function registered by the SPU
>>>>> backend is ever called directly?
>>>>>
>>>>> You mention that this is latent as far as expand_twoval_binop_libfunc
>>>>> is concerned.  AIUI expand_twoval_binop_libfunc implements the ARM/c6x
>>>>> convention and expects the two values to be returned as a pair.
>>>>> It then extracts one half of the pair and discards the other.
>>>>> So my worry is that we're leaving the udivmod entry intact even though
>>>>> the standard __udivmoddi4 doesn't do what expand_twoval_binop_libfunc
>>>>> expects.
>>>>>
>>>>> Would it make sense to set both entries to null and treat __udivmoddi4
>>>>> as a non-optab function?  ARM and c6x could then continue to register
>>>>> their current optab functions and a non-null optab function would
>>>>> indicate a return value pair.
>>>> AFAIU, there are only three targets (c6x, spu, arm) that override
>>>> optab_libfunc for udivmod_optab for following modes:
>>>> ./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, SImode, "__c6xabi_divremu");
>>>> ./c6x/c6x.c:  set_optab_libfunc (udivmod_optab, DImode, "__c6xabi_divremull");
>>>> ./arm/arm.c:  set_optab_libfunc (udivmod_optab, DImode, "__aeabi_uldivmod");
>>>> ./arm/arm.c:  set_optab_libfunc (udivmod_optab, SImode, "__aeabi_uidivmod");
>>>> ./spu/spu.c:  set_optab_libfunc (udivmod_optab, DImode, "__udivmoddi4");
>>>> ./spu/spu.c:  set_optab_libfunc (udivmod_optab, TImode, "__udivmodti4");
>>>>
>>>> Out of these only the arm, and c6x have target-specific divmod libfuncs which
>>>> return <div, mod> pair, while spu merely makes it point to the
>>>> standard functions.
>>>>
>>>> So we could set libfunc entry for udivmod_optab to NULL, thus dropping
>>>> support for generic
>>>> divmod functions (__udivmoddi4, __udivmodti4). For targets that
>>>> require standard divmod libfuncs like __udivmoddi4,
>>>> they could explicitly override  optab_libfunc and set it to
>>>> __udivmoddi4, just as spu does.
>>>>
>>>> However this implies that non-null optab function doesn't necessarily
>>>> follow arm/c6x convention.
>>>> (i686-gcc for instance generates call to libgcc routines
>>>> __udivdi3/__umoddi3 for DImode division/mod operations
>>>> and could profit from divmod transform by calling __udivmoddi4).
>>>
>>> What I meant was that we shouldn't treat udivmoddi4 as an optab function
>>> at all, but handle it with some on-the-side mechanism.  That seems like
>>> quite a natural fit if we handle the fused div/mod operation as an
>>> internal function during gimple.
>> Ah right, thanks for pointing out. So if optab function for [us]divmod_optab
>> is defined, then it must follow the arm/c6x convention ?
>>>
>>> I think the current SPU code is wrong, but it looks like a latent bug.
>>> (Like I say, does the udivmodti4 function that it registers ever
>>> actually get used?  It seems unlikely.)
>>>
>>> In that scenario no other targets should do what SPU does.
>> I am testing the following patch which sets libfunc entries for both
>> sdivmod_optab, udivmod_optab to NULL.
>> This won't change the current (broken) behavior for SPU port since it
>> explicitly overrides optab_libfunc for udivmod_optab
>> and sets it to __udivmoddi4.
>
> Just
>
> -OPTAB_NL(sdivmod_optab, "divmod$a4", UNKNOWN, "divmod", '4', gen_int_libfunc)
> -OPTAB_NL(udivmod_optab, "udivmod$a4", UNKNOWN, "udivmod", '4', gen_int_libfunc)
> +OPTAB_NC(sdivmod_optab, "divmod$a4", UNKNOWN)
> +OPTAB_NC(udivmod_optab, "udivmod$a4", UNKNOWN)
>
> If I remember correctly.
Thanks for the pointers, I verified with the patch,
optab_libfunc (udivmod_optab, DImode) returns NULL for targets that
don't override
libfunc for udivmod_optab like x86_64, and returns correct libfunc for
targets that
override libfunc for udivmod_optab like arm.

The patch would make divmod transform more simpler, since now we require that
the divmod libfunc has a consistent interface identical to arm/c6x convention
(take 2 args and return value <div, mod> pair), so we can get rid of
the target hook
in divmod patch.

However I see one potential issue with SPU port for divmod transform:
So far, it appears the only function that could generate call to __udivmoddi4()
was expand_twoval_binop_libfunc() which was called from expand_divmod().
Maybe that code-path never got triggered for SPU and the issue remained latent.

However with divmod patch, we are introducing another code-path to generate
call to __udivmoddi4(), and since __udivmoddi4() and divmod libfunc conventions
don't agree, this will result in wrong code with the divmod transform on SPU.

We could work around this by SPU port not overriding optab_libfunc for
udivmod_optab,
by removing following lines in spu_init_libfuncs():
set_optab_libfunc (udivmod_optab, DImode, "__udivmoddi4");
set_optab_libfunc (udivmod_optab, TImode, "__udivmodti4");
If that's not desirable, we could disable the divmod transform for spu
for now and revisit the issue later.

The patch passes bootstrap+test on x86_64-unknown-linux-gnu,
aarch64-linux-gnu, ppc64le-linux-gnu, and cross tested on arm*-*-*,
aarch64*-*-*.
Would it be OK to commit ?
I am happy to do more testing if required and if something breaks due
to this patch,
I will revert it immediately.

Thanks,
Prathamesh
>
> Richard.
>
>
>> Thanks,
>> Prathamesh
>>>
>>> Thanks,
>>> Richard

[-- Attachment #2: null-usdivmod-2.diff --]
[-- Type: text/plain, Size: 896 bytes --]

diff --git a/gcc/optabs.def b/gcc/optabs.def
index 8875e30..8a64ce1 100644
--- a/gcc/optabs.def
+++ b/gcc/optabs.def
@@ -116,8 +116,8 @@ OPTAB_NL(ssdiv_optab, "ssdiv$Q$a3", SS_DIV, "ssdiv", '3', gen_signed_fixed_libfu
 OPTAB_NL(udiv_optab, "udiv$I$a3", UDIV, "udiv", '3', gen_int_unsigned_fixed_libfunc)
 OPTAB_NX(udiv_optab, "udiv$Q$a3")
 OPTAB_NL(usdiv_optab, "usdiv$Q$a3", US_DIV, "usdiv", '3', gen_unsigned_fixed_libfunc)
-OPTAB_NL(sdivmod_optab, "divmod$a4", UNKNOWN, "divmod", '4', gen_int_libfunc)
-OPTAB_NL(udivmod_optab, "udivmod$a4", UNKNOWN, "udivmod", '4', gen_int_libfunc)
+OPTAB_NC(sdivmod_optab, "divmod$a4", UNKNOWN) 
+OPTAB_NC(udivmod_optab, "udivmod$a4", UNKNOWN)
 OPTAB_NL(smod_optab, "mod$a3", MOD, "mod", '3', gen_int_libfunc)
 OPTAB_NL(umod_optab, "umod$a3", UMOD, "umod", '3', gen_int_libfunc)
 OPTAB_NL(ftrunc_optab, "ftrunc$F$a2", UNKNOWN, "ftrunc", '2', gen_fp_libfunc)