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

[Prathamesh Kulkarni <prathamesh.kulkarni@linaro.org>]

[-- 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)