From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 28885 invoked by alias); 19 Sep 2016 11:25:15 -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 28860 invoked by uid 89); 19 Sep 2016 11:25:14 -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_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,RCVD_IN_SORBS_SPAM,SPF_PASS,URIBL_RED autolearn=ham version=3.3.2 spammy= X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-wm0-f68.google.com Received: from mail-wm0-f68.google.com (HELO mail-wm0-f68.google.com) (74.125.82.68) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 19 Sep 2016 11:25:04 +0000 Received: by mail-wm0-f68.google.com with SMTP id 133so14403430wmq.2; Mon, 19 Sep 2016 04:25:03 -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:content-transfer-encoding; bh=3B3Uql1fZlZ2nviANeC3sPMqBrcb43GU7c4tRzTFw90=; b=G4NvSMEak8uei5ZC9qi9uCDuqPpJMnWtszEzTHXI9FP4zDOprn1alSfZVnSGu1kapR KwXVI5DVgtjHPRFBHFY8O6d9hv8ppgC4MvsKyGkbENTuO23VTDDjbUlo7/lrlnxcM9uq XVNamQ0Np0U4pj5J5wXSnEf7tVQxOxOPcIWGhwAO0lrmVliQORgGlIubyQQmqUzBomNP enL2AKfOrBA9D78rBHwldtLCjGZ7QSnigR8PTneYzQ57/4RQjiu3vJBnqmESGqAcyrTM UqDolYPrFsyC7XdTvaB3+CU82pwVTMfp1l+sN7oLYfM2Mg+9gBR7hTqdvsuyhXHTtbrc 1g3A== X-Gm-Message-State: AE9vXwOJ+O24s75hbXkme3YYy9yZ/lH2eIPaGM6nhT8M/5ZSTwzroLvFRytc0YLGtyJai3C+MuKcnFUXk2W+Rw== X-Received: by 10.28.142.83 with SMTP id q80mr8424606wmd.92.1474284302038; Mon, 19 Sep 2016 04:25:02 -0700 (PDT) MIME-Version: 1.0 Received: by 10.28.137.129 with HTTP; Mon, 19 Sep 2016 04:25:01 -0700 (PDT) In-Reply-To: <87bmzkb0h5.fsf@hertz.schwinge.homeip.net> References: <20160817154244.GA39270@arm.com> <87h99s53ls.fsf@kepler.schwinge.homeip.net> <87d1kefwgt.fsf@hertz.schwinge.homeip.net> <87twdgb9zi.fsf@hertz.schwinge.homeip.net> <87poo4as2j.fsf@hertz.schwinge.homeip.net> <87bmzkb0h5.fsf@hertz.schwinge.homeip.net> From: Richard Biener Date: Mon, 19 Sep 2016 12:07:00 -0000 Message-ID: Subject: Re: [PR lto/77458] Avoid ICE in offloading with differing _FloatN, _FloatNx types (was: Advice sought for debugging a lto1 ICE (was: Implement C _FloatN, _FloatNx types [version 6])) To: Thomas Schwinge Cc: GCC Patches , Joseph Myers , GCC Development , Jakub Jelinek , Bernd Schmidt Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2016-09/txt/msg01161.txt.bz2 On Mon, Sep 19, 2016 at 1:19 PM, Thomas Schwinge wrote: > Hi! > > On Mon, 19 Sep 2016 10:18:35 +0200, Richard Biener wrote: >> On Fri, Sep 16, 2016 at 3:32 PM, Thomas Schwinge >> wrote: >> > --- gcc/tree-core.h >> > +++ gcc/tree-core.h >> > @@ -553,20 +553,6 @@ enum tree_index { >> > TI_BOOLEAN_FALSE, >> > TI_BOOLEAN_TRUE, >> > >> > - TI_COMPLEX_INTEGER_TYPE, >> > -[...] >> > - TI_COMPLEX_FLOAT128X_TYPE, >> > - >> > TI_FLOAT_TYPE, >> > TI_DOUBLE_TYPE, >> > TI_LONG_DOUBLE_TYPE, >> > @@ -596,6 +582,23 @@ enum tree_index { >> > - TI_FLOATN_NX_TYPE_FIRST \ >> > + 1) >> > >> > + /* Put the complex types after their component types, so that in (s= equential) >> > + tree streaming we can assert that their component types have alr= eady been >> > + handled (see tree-streamer.c:record_common_node). */ >> > + TI_COMPLEX_INTEGER_TYPE, >> > + TI_COMPLEX_FLOAT_TYPE, >> > + TI_COMPLEX_DOUBLE_TYPE, >> > + TI_COMPLEX_LONG_DOUBLE_TYPE, >> > + >> > + TI_COMPLEX_FLOAT16_TYPE, >> > + TI_COMPLEX_FLOATN_NX_TYPE_FIRST =3D TI_COMPLEX_FLOAT16_TYPE, >> > + TI_COMPLEX_FLOAT32_TYPE, >> > + TI_COMPLEX_FLOAT64_TYPE, >> > + TI_COMPLEX_FLOAT128_TYPE, >> > + TI_COMPLEX_FLOAT32X_TYPE, >> > + TI_COMPLEX_FLOAT64X_TYPE, >> > + TI_COMPLEX_FLOAT128X_TYPE, >> > + >> > TI_FLOAT_PTR_TYPE, >> > TI_DOUBLE_PTR_TYPE, >> > TI_LONG_DOUBLE_PTR_TYPE, >> >> If the above change alone fixes your issues then it is fine to commit. > > That alone won't fix the problem, because we'd still have the recursion > in gcc/tree-streamer.c:record_common_node done differently for x86_64 > target and nvptx offload target. Doh - obviously. >> > --- gcc/tree-streamer.c >> > +++ gcc/tree-streamer.c >> > @@ -278,9 +278,23 @@ record_common_node (struct streamer_tree_cache_d = *cache, tree node) >> > streamer_tree_cache_append (cache, node, cache->nodes.length ()); >> > >> > if (POINTER_TYPE_P (node) >> > - || TREE_CODE (node) =3D=3D COMPLEX_TYPE >> > || TREE_CODE (node) =3D=3D ARRAY_TYPE) >> > record_common_node (cache, TREE_TYPE (node)); >> > + else if (TREE_CODE (node) =3D=3D COMPLEX_TYPE) >> > + { >> > + /* Assert that complex types' component types have already been= handled >> > + (and we thus don't need to recurse here). See PR lto/77458. = */ >> > + if (cache->node_map) >> > + gcc_assert (streamer_tree_cache_lookup (cache, TREE_TYPE (node= ), NULL)); >> > + else >> > + { >> > + gcc_assert (cache->nodes.exists ()); >> > + bool found =3D false; >> > + for (unsigned i =3D 0; !found && i < cache->nodes.length ();= ++i) >> > + found =3D true; >> >> hmm, this doesn't actually test anything? ;) > > ;-) Haha, hooray for patch review! > >> > + gcc_assert (found); >> > + } >> > + } >> > else if (TREE_CODE (node) =3D=3D RECORD_TYPE) >> > { >> > /* The FIELD_DECLs of structures should be shared, so that every > >> So I very much like to go forward with this kind of change as well > > OK, good. So, in plain text, we'll make it a requirement that: > integer_types trees must only refer to earlier integer_types trees; > sizetype_tab trees must only refer to integer_types trees, and earlier > sizetype_tab trees; and global_trees must only refer to integer_types > trees, sizetype_tab trees, and earlier global_trees. Yeah, though I'd put sizetypes first. >> (the assert code >> should go to a separate helper function). > > Should this checking be done only in > gcc/tree-streamer.c:record_common_node, or should generally Yes. > gcc/tree-streamer.c:streamer_tree_cache_append check/assert that such > recursive trees are already present in the cache? And generally do that, > or "if (flag_checking)" only? I think we should restrict it to flag_checking only because in general viol= ating it is harmless plus we never know what happens on all targets/frontend/flag= (!) combinations. > >> Did you try it on more than just >> the complex type case? > > Not yet, but now that you have approved the general concept, I'll look > into that. > > Here's the current patch with the assertion condition fixed, but still > for complex types only. OK for trunk already? Ok with the checking blob moved to a helper function, bool common_node_recorded_p (cache, node) and its body guarded with if (flag_checking). [looks to me we miss handling of vector type components alltogether, maybe there are no global vector type trees ...] Thanks, Richard. > --- gcc/tree-core.h > +++ gcc/tree-core.h > @@ -553,20 +553,6 @@ enum tree_index { > TI_BOOLEAN_FALSE, > TI_BOOLEAN_TRUE, > > - TI_COMPLEX_INTEGER_TYPE, > - TI_COMPLEX_FLOAT_TYPE, > - TI_COMPLEX_DOUBLE_TYPE, > - TI_COMPLEX_LONG_DOUBLE_TYPE, > - > - TI_COMPLEX_FLOAT16_TYPE, > - TI_COMPLEX_FLOATN_NX_TYPE_FIRST =3D TI_COMPLEX_FLOAT16_TYPE, > - TI_COMPLEX_FLOAT32_TYPE, > - TI_COMPLEX_FLOAT64_TYPE, > - TI_COMPLEX_FLOAT128_TYPE, > - TI_COMPLEX_FLOAT32X_TYPE, > - TI_COMPLEX_FLOAT64X_TYPE, > - TI_COMPLEX_FLOAT128X_TYPE, > - > TI_FLOAT_TYPE, > TI_DOUBLE_TYPE, > TI_LONG_DOUBLE_TYPE, > @@ -596,6 +582,23 @@ enum tree_index { > - TI_FLOATN_NX_TYPE_FIRST \ > + 1) > > + /* Put the complex types after their component types, so that in (sequ= ential) > + tree streaming we can assert that their component types have alread= y been > + handled (see tree-streamer.c:record_common_node). */ > + TI_COMPLEX_INTEGER_TYPE, > + TI_COMPLEX_FLOAT_TYPE, > + TI_COMPLEX_DOUBLE_TYPE, > + TI_COMPLEX_LONG_DOUBLE_TYPE, > + > + TI_COMPLEX_FLOAT16_TYPE, > + TI_COMPLEX_FLOATN_NX_TYPE_FIRST =3D TI_COMPLEX_FLOAT16_TYPE, > + TI_COMPLEX_FLOAT32_TYPE, > + TI_COMPLEX_FLOAT64_TYPE, > + TI_COMPLEX_FLOAT128_TYPE, > + TI_COMPLEX_FLOAT32X_TYPE, > + TI_COMPLEX_FLOAT64X_TYPE, > + TI_COMPLEX_FLOAT128X_TYPE, > + > TI_FLOAT_PTR_TYPE, > TI_DOUBLE_PTR_TYPE, > TI_LONG_DOUBLE_PTR_TYPE, > --- gcc/tree-streamer.c > +++ gcc/tree-streamer.c > @@ -278,9 +278,26 @@ record_common_node (struct streamer_tree_cache_d *ca= che, tree node) > streamer_tree_cache_append (cache, node, cache->nodes.length ()); > > if (POINTER_TYPE_P (node) > - || TREE_CODE (node) =3D=3D COMPLEX_TYPE > || TREE_CODE (node) =3D=3D ARRAY_TYPE) > record_common_node (cache, TREE_TYPE (node)); > + else if (TREE_CODE (node) =3D=3D COMPLEX_TYPE) > + { > + /* Assert that a complex type's component type (node_type) has been > + handled already (and we thus don't need to recurse here). See PR > + lto/77458. */ > + tree node_type =3D TREE_TYPE (node); > + if (cache->node_map) > + gcc_assert (streamer_tree_cache_lookup (cache, node_type, NULL)); > + else > + { > + gcc_assert (cache->nodes.exists ()); > + bool found =3D false; > + for (unsigned i =3D 0; !found && i < cache->nodes.length (); ++= i) > + if (cache->nodes[i] =3D=3D node_type) > + found =3D true; > + gcc_assert (found); > + } > + } > else if (TREE_CODE (node) =3D=3D RECORD_TYPE) > { > /* The FIELD_DECLs of structures should be shared, so that every > > > Gr=C3=BC=C3=9Fe > Thomas