* [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def @ 2016-09-09 22:05 Prathamesh Kulkarni 2016-09-15 0:21 ` Richard Sandiford 0 siblings, 1 reply; 8+ messages in thread From: Prathamesh Kulkarni @ 2016-09-09 22:05 UTC (permalink / raw) To: gcc Patches, Richard Sandiford 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def 2016-09-09 22:05 [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def Prathamesh Kulkarni @ 2016-09-15 0:21 ` Richard Sandiford 2016-09-15 3:08 ` Richard Sandiford 0 siblings, 1 reply; 8+ messages in thread From: Richard Sandiford @ 2016-09-15 0:21 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, 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: ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def 2016-09-15 0:21 ` Richard Sandiford @ 2016-09-15 3:08 ` Richard Sandiford 2016-09-15 10:53 ` Prathamesh Kulkarni 0 siblings, 1 reply; 8+ messages in thread From: Richard Sandiford @ 2016-09-15 3:08 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def 2016-09-15 3:08 ` Richard Sandiford @ 2016-09-15 10:53 ` Prathamesh Kulkarni 2016-09-15 12:12 ` Richard Sandiford 0 siblings, 1 reply; 8+ messages in thread From: Prathamesh Kulkarni @ 2016-09-15 10:53 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Richard Sandiford, rdsandiford 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def 2016-09-15 10:53 ` Prathamesh Kulkarni @ 2016-09-15 12:12 ` Richard Sandiford 2016-09-15 12:16 ` Prathamesh Kulkarni 0 siblings, 1 reply; 8+ messages in thread From: Richard Sandiford @ 2016-09-15 12:12 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def 2016-09-15 12:12 ` Richard Sandiford @ 2016-09-15 12:16 ` Prathamesh Kulkarni 2016-09-15 12:42 ` Richard Biener 0 siblings, 1 reply; 8+ messages in thread From: Prathamesh Kulkarni @ 2016-09-15 12:16 UTC (permalink / raw) To: Prathamesh Kulkarni, gcc Patches, Richard Sandiford [-- 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def 2016-09-15 12:16 ` Prathamesh Kulkarni @ 2016-09-15 12:42 ` Richard Biener 2016-09-16 11:31 ` Prathamesh Kulkarni 0 siblings, 1 reply; 8+ messages in thread From: Richard Biener @ 2016-09-15 12:42 UTC (permalink / raw) To: Prathamesh Kulkarni; +Cc: gcc Patches, Richard Sandiford 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def 2016-09-15 12:42 ` Richard Biener @ 2016-09-16 11:31 ` Prathamesh Kulkarni 0 siblings, 0 replies; 8+ messages in thread From: Prathamesh Kulkarni @ 2016-09-16 11:31 UTC (permalink / raw) To: Richard Biener; +Cc: gcc Patches, Richard Sandiford [-- 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) ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-09-16 11:02 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-09 22:05 [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def Prathamesh Kulkarni 2016-09-15 0:21 ` Richard Sandiford 2016-09-15 3:08 ` Richard Sandiford 2016-09-15 10:53 ` Prathamesh Kulkarni 2016-09-15 12:12 ` Richard Sandiford 2016-09-15 12:16 ` Prathamesh Kulkarni 2016-09-15 12:42 ` Richard Biener 2016-09-16 11:31 ` Prathamesh Kulkarni
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).