From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52f.google.com (mail-pg1-x52f.google.com [IPv6:2607:f8b0:4864:20::52f]) by sourceware.org (Postfix) with ESMTPS id 974063858007 for ; Wed, 25 Aug 2021 23:16:09 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 974063858007 Received: by mail-pg1-x52f.google.com with SMTP id n18so1208005pgm.12 for ; Wed, 25 Aug 2021 16:16:09 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language; bh=8zx4j/R2g1BNDHHKINwMuPTYLu2ERe9fF2KyeXwgZKw=; b=EC7+0K6yOXO75zQeBWOrMb2IjLUd8DtUzpsC7K5+IVIsqoqxx5FoVHsXGE3Rd/5veR f76fyaepjusKWiLKXLABkNPHOJr/mI9oA4edUmOjotofXi1br+xIH4tR4jTQIeJjWi7J Kvdvxk3Z39l4cEnbP3GP0OLBHu7lkP4ggr0CMB8u1T22oTjeIozxf3muJCcLT+8CqyTo iVU4CtGAA82N+bctCg4oxUQ7naWaxfSm8HxDOPZ80xpTDWqrEbk98fTJYxDM+IYyTaTi +C+K49LunKOfQSGNwwI/s9nOHUl8oPS6ZHb/aLGBadDQA8JBdNIvmo1Q/YO6iu7DN7S9 wc4w== X-Gm-Message-State: AOAM530Da0Rs+0EJEFzMP+IoBindMUR0PD6+kFSPupd6ilufaTCyxOyh neGOKyiAnjTJ/7JbyLtAl69s4Jy0fUBfIw== X-Google-Smtp-Source: ABdhPJxP6N6b3S6rj9pDJGWZ5gZgnKdSd4OIyxjAIgpf/Rl7E41hYFnY7jKy8bfOxRt+LeLUG7o0jg== X-Received: by 2002:a05:6a00:1748:b0:3e2:ace4:82b7 with SMTP id j8-20020a056a00174800b003e2ace482b7mr705647pfc.56.1629933368143; Wed, 25 Aug 2021 16:16:08 -0700 (PDT) Received: from [172.31.0.175] (c-98-202-48-222.hsd1.ut.comcast.net. [98.202.48.222]) by smtp.gmail.com with ESMTPSA id z7sm6394675pjr.42.2021.08.25.16.16.07 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 Aug 2021 16:16:07 -0700 (PDT) Subject: Re: [PATCH] Make sure we're playing with integral modes before call extract_integral_bit_field. To: Hongtao Liu , Richard Biener Cc: Richard Sandiford , liuhongt , Richard Biener via Gcc-patches References: <20210806033228.3270579-1-hongtao.liu@intel.com> From: Jeff Law Message-ID: <3ed030df-d125-865f-0d75-1c8e8f820daa@gmail.com> Date: Wed, 25 Aug 2021 17:16:06 -0600 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.13.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US X-Spam-Status: No, score=-9.5 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, GIT_PATCH_0, HTML_MESSAGE, KAM_SHORT, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 25 Aug 2021 23:16:20 -0000 On 8/24/2021 3:44 AM, Hongtao Liu via Gcc-patches wrote: > On Tue, Aug 24, 2021 at 5:40 PM Hongtao Liu wrote: >> On Tue, Aug 17, 2021 at 9:52 AM Hongtao Liu wrote: >>> On Mon, Aug 9, 2021 at 4:34 PM Hongtao Liu wrote: >>>> On Fri, Aug 6, 2021 at 7:27 PM Richard Biener via Gcc-patches >>>> wrote: >>>>> On Fri, Aug 6, 2021 at 11:05 AM Richard Sandiford >>>>> wrote: >>>>>> Richard Biener via Gcc-patches writes: >>>>>>> On Fri, Aug 6, 2021 at 5:32 AM liuhongt wrote: >>>>>>>> Hi: >>>>>>>> --- >>>>>>>> OK, I think sth is amiss here upthread. insv/extv do look like they >>>>>>>> are designed >>>>>>>> to work on integer modes (but docs do not say anything about this here). >>>>>>>> In fact the caller of extract_bit_field_using_extv is named >>>>>>>> extract_integral_bit_field. Of course nothing seems to check what kind of >>>>>>>> modes we're dealing with, but we're for example happily doing >>>>>>>> expand_shift in 'mode'. In the extract_integral_bit_field call 'mode' is >>>>>>>> some integer mode and op0 is HFmode? From the above I get it's >>>>>>>> the other way around? In that case we should wrap the >>>>>>>> call to extract_integral_bit_field, extracting in an integer mode with the >>>>>>>> same size as 'mode' and then converting the result as (subreg:HF (reg:HI ...)). >>>>>>>> --- >>>>>>>> This is a separate patch as a follow up of upper comments. >>>>>>>> >>>>>>>> gcc/ChangeLog: >>>>>>>> >>>>>>>> * expmed.c (extract_bit_field_1): Wrap the call to >>>>>>>> extract_integral_bit_field, extracting in an integer mode with >>>>>>>> the same size as 'tmode' and then converting the result >>>>>>>> as (subreg:tmode (reg:imode)). >>>>>>>> >>>>>>>> gcc/testsuite/ChangeLog: >>>>>>>> * gcc.target/i386/float16-5.c: New test. >>>>>>>> --- >>>>>>>> gcc/expmed.c | 19 +++++++++++++++++++ >>>>>>>> gcc/testsuite/gcc.target/i386/float16-5.c | 12 ++++++++++++ >>>>>>>> 2 files changed, 31 insertions(+) >>>>>>>> create mode 100644 gcc/testsuite/gcc.target/i386/float16-5.c >>>>>>>> >>>>>>>> diff --git a/gcc/expmed.c b/gcc/expmed.c >>>>>>>> index 3143f38e057..72790693ef0 100644 >>>>>>>> --- a/gcc/expmed.c >>>>>>>> +++ b/gcc/expmed.c >>>>>>>> @@ -1850,6 +1850,25 @@ extract_bit_field_1 (rtx str_rtx, poly_uint64 bitsize, poly_uint64 bitnum, >>>>>>>> op0_mode = opt_scalar_int_mode (); >>>>>>>> } >>>>>>>> >>>>>>>> + /* Make sure we are playing with integral modes. Pun with subregs >>>>>>>> + if we aren't. When tmode is HFmode, op0 is SImode, there will be ICE >>>>>>>> + in extract_integral_bit_field. */ >>>>>>>> + if (int_mode_for_mode (tmode).exists (&imode) >>>>>>> check !INTEGRAL_MODE_P (tmode) before, that should be slightly >>>>>>> cheaper. Then imode should always be != tmode. Maybe >>>>>>> even GET_MDOE_CLASS (tmode) != MODE_INT since I'm not sure >>>>>>> how it behaves for composite modes. >>>>>>> >>>>>>> Of course the least surprises would happen when we restrict this >>>>>>> to FLOAT_MODE_P (tmode). >>>>>>> >>>>>>> Richard - any preferences? >>>>>> If the bug is that extract_integral_bit_field is being called with >>>>>> a non-integral mode parameter, then it looks odd that we can still >>>>>> fall through to it without an integral mode (when exists is false). >>>>>> >>>>>> If calling extract_integral_bit_field without an integral mode is >>>>>> a bug then I think we should have: >>>>>> >>>>>> int_mode_for_mode (mode).require () >>>>>> >>>>>> whenever mode is not already SCALAR_INT_MODE_P/is_a. >>>>>> Ideally we'd make the mode parameter scalar_int_mode too. >>>>>> >>>>>> extract_integral_bit_field currently has: >>>>>> >>>>>> /* Find a correspondingly-sized integer field, so we can apply >>>>>> shifts and masks to it. */ >>>>>> scalar_int_mode int_mode; >>>>>> if (!int_mode_for_mode (tmode).exists (&int_mode)) >>>>>> /* If this fails, we should probably push op0 out to memory and then >>>>>> do a load. */ >>>>>> int_mode = int_mode_for_mode (mode).require (); >>>>>> >>>>>> which would seem to be redundant after this change. >>>>> I'm not sure what exactly the bug is, but extract_integral_bit_field ends >>>>> up creating a lowpart subreg that's not allowed and that ICEs (and I >>>>> can't see a way to check beforehand). So it seems to me at least >>>>> part of that function doesn't expect non-integral extraction modes. >>>>> >>>>> But who knows - the code is older than I am (OK, not, but older than >>>>> my involvment in GCC ;)) >>>>> >>>> How about attached patch w/ below changelog >>>> >>>> gcc/ChangeLog: >>>> >>>> * expmed.c (extract_bit_field_1): Make sure we're playing with >>>> integral modes before call extract_integral_bit_field. >>>> (extract_integral_bit_field): Add a parameter of type >>>> scalar_int_mode which corresponds to of tmode. >>>> And call extract_and_convert_fixed_bit_field instead of >>>> extract_fixed_bit_field and convert_extracted_bit_field. >>>> (extract_and_convert_fixed_bit_field): New function, it's a >>>> combination of extract_fixed_bit_field and >>>> convert_extracted_bit_field. >>>> >>>> gcc/testsuite/ChangeLog: >>>> * gcc.target/i386/float16-5.c: New test. >>>> >>> I'd like to ping this patch, or maybe we can use the patch before with >>> richi's comments. >> Rebased and ping^2, there are plenty of avx512fp16 patches blocked by >> this patch, i'd like someone to help review this patch. >> > Please ignore the former attached patch, should be the patch attached here. >>>>> Richard. >>>>> >>>>>>>> + && imode != tmode >>>>>>>> + && imode != GET_MODE (op0)) >>>>>>>> + { >>>>>>>> + rtx ret = extract_integral_bit_field (op0, op0_mode, >>>>>>>> + bitsize.to_constant (), >>>>>>>> + bitnum.to_constant (), unsignedp, >>>>>>>> + NULL, imode, imode, >>>>>>>> + reverse, fallback_p); >>>>>>>> + gcc_assert (ret); >>>>>>>> + >>>>>>>> + if (!REG_P (ret)) >>>>>>>> + ret = force_reg (imode, ret); >>>>>>>> + return gen_lowpart_SUBREG (tmode, ret); >>>>>>>> + } >>>>>>>> + >>>>>>>> /* It's possible we'll need to handle other cases here for >>>>>>>> polynomial bitnum and bitsize. */ >>>>>> Minor nit, but since the code is using to_constant, it should go after >>>>>> this comment rather than before it. >>>>>> >>>>>> Thanks, >>>>>> Richard >>>>>> >>>>>>>> diff --git a/gcc/testsuite/gcc.target/i386/float16-5.c b/gcc/testsuite/gcc.target/i386/float16-5.c >>>>>>>> new file mode 100644 >>>>>>>> index 00000000000..ebc0af1490b >>>>>>>> --- /dev/null >>>>>>>> +++ b/gcc/testsuite/gcc.target/i386/float16-5.c >>>>>>>> @@ -0,0 +1,12 @@ >>>>>>>> +/* { dg-do compile } */ >>>>>>>> +/* { dg-options "-msse2 -O2" } */ >>>>>>>> +_Float16 >>>>>>>> +foo (int a) >>>>>>>> +{ >>>>>>>> + union { >>>>>>>> + int a; >>>>>>>> + _Float16 b; >>>>>>>> + }c; >>>>>>>> + c.a = a; >>>>>>>> + return c.b; >>>>>>>> +} >>>>>>>> -- >>>>>>>> 2.27.0 >>>>>>>> >>>> >>>> >>>> -- >>>> BR, >>>> Hongtao >>> >>> >>> -- >>> BR, >>> Hongtao >> >> >> -- >> BR, >> Hongtao > > > > 0001-Make-sure-we-re-playing-with-integral-modes-before-c.patch > > From 9c77ac15e69b567156a82debe45e3ced10df1110 Mon Sep 17 00:00:00 2001 > From: liuhongt > Date: Fri, 6 Aug 2021 10:18:43 +0800 > Subject: [PATCH] Make sure we're playing with integral modes before call > extract_integral_bit_field. > > gcc/ChangeLog: > > * expmed.c (extract_bit_field_1): Make sure we're playing with > integral modes before call extract_integral_bit_field. > (extract_integral_bit_field): Add a parameter of type > scalar_int_mode which corresponds to of tmode. > And call extract_and_convert_fixed_bit_field instead of > extract_fixed_bit_field and convert_extracted_bit_field. > (extract_and_convert_fixed_bit_field): New function, it's a > combination of extract_fixed_bit_field and > convert_extracted_bit_field. > > gcc/testsuite/ChangeLog: > * gcc.target/i386/float16-5.c: New test. I bet this is all getting triggered due to the introduction of HFmode.  Wrapping with a subreg to get an integral mode may work, but I'd be more comfortable if we had other instances where we knew wrapping an SF/DF mode with SI/DI was enough to make all this code safe.  I fear we're just pushing the bug down in one spot and it's going to pop up elsewhere. Another approach would be to force the object into memory, but I suspect y'all don't want to do that ;-) So in the end, it may be reasonable, but I wouldn't be surprised if we trip over more problems in this code with FP modes. jeff