On 15 September 2016 at 17:53, Richard Biener
<richard.guent...@gmail.com> wrote:
> On Thu, Sep 15, 2016 at 1:21 PM, Prathamesh Kulkarni
> <prathamesh.kulka...@linaro.org> wrote:
>> On 15 September 2016 at 16:31, Richard Sandiford
>> <richard.sandif...@arm.com> wrote:
>>> Prathamesh Kulkarni <prathamesh.kulka...@linaro.org> writes:
>>>> On 15 September 2016 at 04:21, Richard Sandiford
>>>> <rdsandif...@googlemail.com> wrote:
>>>>> Richard Sandiford <rdsandif...@googlemail.com> writes:
>>>>>> Prathamesh Kulkarni <prathamesh.kulka...@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
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*-*-*,
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.

> Richard.
>> Thanks,
>> Prathamesh
>>> Thanks,
>>> Richard
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', 
 OPTAB_NL(udiv_optab, "udiv$I$a3", UDIV, "udiv", '3', 
 OPTAB_NX(udiv_optab, "udiv$Q$a3")
 OPTAB_NL(usdiv_optab, "usdiv$Q$a3", US_DIV, "usdiv", '3', 
-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)

Reply via email to