From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29633 invoked by alias); 16 Sep 2016 11:02:41 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 29621 invoked by uid 89); 16 Sep 2016 11:02:40 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=2.1 required=5.0 tests=AWL,BAYES_50,LIKELY_SPAM_BODY,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=no version=3.3.2 spammy=richardsandifordarmcom, richard.sandiford@arm.com, sk:gen_int, ftruncfa2 X-HELO: mail-it0-f41.google.com Received: from mail-it0-f41.google.com (HELO mail-it0-f41.google.com) (209.85.214.41) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 16 Sep 2016 11:02:30 +0000 Received: by mail-it0-f41.google.com with SMTP id r192so13000947ita.0 for ; Fri, 16 Sep 2016 04:02:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=3tGlRiL0U1Ajbv1RvdAu7wyV1VWliyUl/UgK9zKgrlk=; b=mc8q30N9pGL9o+IC8UGUH63EBQ8rOMoiB3xjFiMEkh9xK4JCoqUufi5I/r461BWKsh zKlNwuUThk8Ec0wXm8eNkzx/jldfmJwV3vOipoOceTM0ZzM8c3e1yabGEXDScLdDaTs2 moVCGrbJBcGn4lgguWtMRzMfoBifi07mHNhDTgW39Oz2Y3VYDWiAxum4ww/9mzhtCf9B t/InORF+LVgY4cTDGVkWBsl6MFL32Jg9kAUJ8Qa/5fTcnk/3OdY2T5naCW+8UMe1t4V5 QQFGLc5bh+FnPYLjmHdDv8MTOfbpj3dRs2P7pA5S25POSb9gztuqvlpIChlvjzBCUXza kSKg== X-Gm-Message-State: AE9vXwMl/fqac43NhQGUKJmku/5fqgkwm6rsPNmiqX2DV41OKXNkuiimxYU6g3H+JAhMzMU0/Bmjo4LGd9cbL3c2 X-Received: by 10.36.254.197 with SMTP id w188mr5149933ith.6.1474023748892; Fri, 16 Sep 2016 04:02:28 -0700 (PDT) MIME-Version: 1.0 Received: by 10.36.81.85 with HTTP; Fri, 16 Sep 2016 04:02:28 -0700 (PDT) In-Reply-To: References: <87y42urtza.fsf@googlemail.com> <87twdirt7c.fsf@googlemail.com> <878tut77fg.fsf@e105548-lin.cambridge.arm.com> From: Prathamesh Kulkarni Date: Fri, 16 Sep 2016 11:31:00 -0000 Message-ID: Subject: Re: [PING] set libfunc entry for sdivmod_optab to NULL in optabs.def To: Richard Biener Cc: gcc Patches , Richard Sandiford Content-Type: multipart/mixed; boundary=94eb2c049406f1e422053c9de5c6 X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg01040.txt.bz2 --94eb2c049406f1e422053c9de5c6 Content-Type: text/plain; charset=UTF-8 Content-length: 7492 On 15 September 2016 at 17:53, Richard Biener wrote: > On Thu, Sep 15, 2016 at 1:21 PM, Prathamesh Kulkarni > wrote: >> On 15 September 2016 at 16:31, Richard Sandiford >> wrote: >>> Prathamesh Kulkarni writes: >>>> On 15 September 2016 at 04:21, Richard Sandiford >>>> wrote: >>>>> Richard Sandiford writes: >>>>>> Prathamesh Kulkarni 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 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 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 --94eb2c049406f1e422053c9de5c6 Content-Type: text/plain; charset=US-ASCII; name="null-usdivmod-2.diff" Content-Disposition: attachment; filename="null-usdivmod-2.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_it5nmsrr0 Content-length: 1216 ZGlmZiAtLWdpdCBhL2djYy9vcHRhYnMuZGVmIGIvZ2NjL29wdGFicy5kZWYK aW5kZXggODg3NWUzMC4uOGE2NGNlMSAxMDA2NDQKLS0tIGEvZ2NjL29wdGFi cy5kZWYKKysrIGIvZ2NjL29wdGFicy5kZWYKQEAgLTExNiw4ICsxMTYsOCBA QCBPUFRBQl9OTChzc2Rpdl9vcHRhYiwgInNzZGl2JFEkYTMiLCBTU19ESVYs ICJzc2RpdiIsICczJywgZ2VuX3NpZ25lZF9maXhlZF9saWJmdQogT1BUQUJf TkwodWRpdl9vcHRhYiwgInVkaXYkSSRhMyIsIFVESVYsICJ1ZGl2IiwgJzMn LCBnZW5faW50X3Vuc2lnbmVkX2ZpeGVkX2xpYmZ1bmMpCiBPUFRBQl9OWCh1 ZGl2X29wdGFiLCAidWRpdiRRJGEzIikKIE9QVEFCX05MKHVzZGl2X29wdGFi LCAidXNkaXYkUSRhMyIsIFVTX0RJViwgInVzZGl2IiwgJzMnLCBnZW5fdW5z aWduZWRfZml4ZWRfbGliZnVuYykKLU9QVEFCX05MKHNkaXZtb2Rfb3B0YWIs ICJkaXZtb2QkYTQiLCBVTktOT1dOLCAiZGl2bW9kIiwgJzQnLCBnZW5faW50 X2xpYmZ1bmMpCi1PUFRBQl9OTCh1ZGl2bW9kX29wdGFiLCAidWRpdm1vZCRh NCIsIFVOS05PV04sICJ1ZGl2bW9kIiwgJzQnLCBnZW5faW50X2xpYmZ1bmMp CitPUFRBQl9OQyhzZGl2bW9kX29wdGFiLCAiZGl2bW9kJGE0IiwgVU5LTk9X TikgCitPUFRBQl9OQyh1ZGl2bW9kX29wdGFiLCAidWRpdm1vZCRhNCIsIFVO S05PV04pCiBPUFRBQl9OTChzbW9kX29wdGFiLCAibW9kJGEzIiwgTU9ELCAi bW9kIiwgJzMnLCBnZW5faW50X2xpYmZ1bmMpCiBPUFRBQl9OTCh1bW9kX29w dGFiLCAidW1vZCRhMyIsIFVNT0QsICJ1bW9kIiwgJzMnLCBnZW5faW50X2xp YmZ1bmMpCiBPUFRBQl9OTChmdHJ1bmNfb3B0YWIsICJmdHJ1bmMkRiRhMiIs IFVOS05PV04sICJmdHJ1bmMiLCAnMicsIGdlbl9mcF9saWJmdW5jKQo= --94eb2c049406f1e422053c9de5c6--