From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 109278 invoked by alias); 29 Jun 2017 10:44:50 -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 109245 invoked by uid 89); 29 Jun 2017 10:44:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-9.7 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,RCVD_IN_DNSWL_NONE,RCVD_IN_SORBS_SPAM,SPF_PASS autolearn=ham version=3.3.2 spammy= X-HELO: mail-wm0-f50.google.com Received: from mail-wm0-f50.google.com (HELO mail-wm0-f50.google.com) (74.125.82.50) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 29 Jun 2017 10:44:48 +0000 Received: by mail-wm0-f50.google.com with SMTP id w126so78128207wme.0 for ; Thu, 29 Jun 2017 03:44:47 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to; bh=z/W/z4/CW62unfgUO916iPDE/sIbqZ6IYo9L3Z71H7g=; b=rjAfERR20DSQFumJWcRqbUXZWFdF3kJJswjykL8TDTwf6CTd0UKMCqPpmSwbiq9/ur y/DHfRM8BshW7YS6/lBbCp/S69j3+uRb6zs5EIay1C1p101GJuJMTwXSR/i9vltMPhcS gPValbESxzib03stor+Bv4AUD1okXrtoJxs29dR0huYHCDBtfXjijKwuWbc3MEvwPI4C s+Sjz5XtUTAUJnlNt/vgjAiK/b5ewyKYLFe22jOTt5KyqUFEeEqcXWv8XXXgfjWT3EPZ g10UjSKM1mNnsQbmm+LASnY+nsXG11QDZJszEWCfGcIyaacm0bL/O/aFosSm6X+RIVqY BAKw== X-Gm-Message-State: AKS2vOxEc9gNrI/XAgjt9XPrn1vEzU4iS3NdI10EJW/qG60Ciiec+RB6 514UC5quRskjWxqcvL1w7NfY4zD85AZE X-Received: by 10.28.54.65 with SMTP id d62mr10322925wma.85.1498733085890; Thu, 29 Jun 2017 03:44:45 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.111.196 with HTTP; Thu, 29 Jun 2017 03:44:45 -0700 (PDT) In-Reply-To: <87vang6y0j.fsf_-_@googlemail.com> References: <87k244z2c5.fsf@linaro.org> <8760fmgb90.fsf@linaro.org> <874lv36lww.fsf@linaro.org> <87zicv55p0.fsf@linaro.org> <87vang6y0j.fsf_-_@googlemail.com> From: Richard Biener Date: Thu, 29 Jun 2017 10:44:00 -0000 Message-ID: Subject: Re: [v2] PR81136: ICE from inconsistent DR_MISALIGNMENTs To: Richard Biener , GCC Patches , Richard Sandiford Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2017-06/txt/msg02248.txt.bz2 On Wed, Jun 28, 2017 at 3:29 PM, Richard Sandiford wrote: > Richard Biener writes: >> On Mon, Jun 26, 2017 at 1:50 PM, Richard Sandiford >> wrote: >>> Richard Biener writes: >>>> On Mon, Jun 26, 2017 at 1:14 PM, Richard Sandiford >>>> wrote: >>>>> I don't think the problem is the lack of a cap. In the test case we >>>>> see that: >>>>> >>>>> 1. B is known at compile time to be X * vecsize + Y when considered in >>>>> isolation, because the base alignment derived from its DR_REF >= vecsize. >>>>> So DR_MISALIGNMENT (B) == Y. >>>>> >>>>> 2. A's misalignment wrt vecsize is not known at compile time when >>>>> considered in isolation, because no useful base alignment can be >>>>> derived from its DR_REF. (The DR_REF is to a plain int rather than >>>>> to a structure with a high alignment.) So DR_MISALIGNMENT (A) == -1. >>>>> >>>>> 3. A and B when considered as a pair trivially have the same misalignment >>>>> wrt vecsize, for the reasons above. >>>>> >>>>> Each of these results is individually correct. The problem is that the >>>>> assert is conflating two things: it's saying that if we know two datarefs >>>>> have the same misaligment, we must either be able to calculate a >>>>> compile-time misalignment for both datarefs in isolation, or we must >>>>> fail to calculate a compile-time misalignment for both datarefs in >>>>> isolation. That isn't true: it's valid to have situations in which the >>>>> compile-time misalignment is known for one dataref in isolation but not >>>>> for the other. >>>> >>>> True. So the assert should then become >>>> >>>> gcc_assert (! known_alignment_for_access_p (dr) >>>> || DR_MISALIGNMENT (dr) / dr_size == >>>> DR_MISALIGNMENT (dr_peel) / dr_peel_size); >>>> >>>> ? >>> >>> I think it would need to be: >>> >>> gcc_assert (!known_alignment_for_access_p (dr) >>> || !known_alignment_for_access_p (dr_peel) >>> || (DR_MISALIGNMENT (dr) / dr_size >>> == DR_MISALIGNMENT (dr_peel) / dr_peel_size)); >> >> I think for !known_alignment_for_access_p (dr_peel) the assert doesn't make >> any sense (DR_MISALIGNMENT is -1), so yes, you are right. >> >>> But yeah, that would work too. The idea with the assert in the patch was >>> that for unconditional references we probably *do* want to try to compute >>> the same compile-time misalignment, but for optimisation reasons rather >>> than correctness. Maybe that's more properly a gcc_checking_assert >>> though, since nothing goes wrong if it fails. So perhaps: >> >> We shouldn't have asserts for optimization reasons, even with checking >> IMHO. > > OK. > >>> gcc_checking_assert (DR_IS_CONDITIONAL_IN_STMT (dr) >>> || DR_IS_CONDITIONAL_IN_STMT (dr_peel) >>> || (known_alignment_for_access_p (dr) >>> == known_alignment_for_access_p (dr_peel))); >>> >>> as a follow-on assert. >>> >>> Should I split it into two patches, one to change the gcc_assert and >>> another to add the optimisation? >> >> Yes please. > > Here's the patch to relax the assert. I'll post the rest in a new thread. > > Tested as before. OK to install? Ok. Richard. > Thanks, > Richard > > > 2017-06-28 Richard Sandiford > > gcc/ > PR tree-optimization/81136 > * tree-vect-data-refs.c (vect_update_misalignment_for_peel): Only > assert that two references with the same misalignment have the same > compile-time misalignment if those compile-time misalignments > are known. > > gcc/testsuite/ > PR tree-optimization/81136 > * gcc.dg/vect/pr81136.c: New test. > > Index: gcc/tree-vect-data-refs.c > =================================================================== > --- gcc/tree-vect-data-refs.c 2017-06-26 19:41:19.549571836 +0100 > +++ gcc/tree-vect-data-refs.c 2017-06-28 14:25:58.811888377 +0100 > @@ -906,8 +906,10 @@ vect_update_misalignment_for_peel (struc > { > if (current_dr != dr) > continue; > - gcc_assert (DR_MISALIGNMENT (dr) / dr_size == > - DR_MISALIGNMENT (dr_peel) / dr_peel_size); > + gcc_assert (!known_alignment_for_access_p (dr) > + || !known_alignment_for_access_p (dr_peel) > + || (DR_MISALIGNMENT (dr) / dr_size > + == DR_MISALIGNMENT (dr_peel) / dr_peel_size)); > SET_DR_MISALIGNMENT (dr, 0); > return; > } > Index: gcc/testsuite/gcc.dg/vect/pr81136.c > =================================================================== > --- /dev/null 2017-06-28 07:28:02.991792729 +0100 > +++ gcc/testsuite/gcc.dg/vect/pr81136.c 2017-06-28 14:25:58.810888422 +0100 > @@ -0,0 +1,16 @@ > +/* { dg-do compile } */ > + > +struct __attribute__((aligned (32))) > +{ > + char misaligner; > + int foo[100]; > + int bar[100]; > +} *a; > + > +void > +fn1 (int n) > +{ > + int *b = a->foo; > + for (int i = 0; i < n; i++) > + a->bar[i] = b[i]; > +}