From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by sourceware.org (Postfix) with ESMTPS id 528D63858D20 for ; Fri, 27 Jan 2023 09:49:39 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 528D63858D20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=suse.de Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=suse.de Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 371EE201D2; Fri, 27 Jan 2023 09:49:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_rsa; t=1674812978; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=qtI+J49rcY0am9+hTeM4il4ZXPmZDZ9zHDAMYKLFAPo=; b=jC+okuboV6/KW7nSpHkcI+iiGOTKI004N81jmQxVKOnSKIznJnruOeFBP+iVey0sMw96c5 lLrym+g111kclf0paIJQ2pXFLUT3FL8CXUUfcpn3NDn0dh92KUJ2kPzlnagoN1LFPuOpBM lPZDXtxGRYxAbaNOC7oLo5YfilTNw84= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.de; s=susede2_ed25519; t=1674812978; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=qtI+J49rcY0am9+hTeM4il4ZXPmZDZ9zHDAMYKLFAPo=; b=3xfDzCgjzqiy2asqZsSMEFDB+BGoRtW3LP/m3eePnHtAvHXze5iEJzsD+aLW7DcnnRydag MdMsT8JnuYty8gDQ== Received: from wotan.suse.de (wotan.suse.de [10.160.0.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 2E0BB2C141; Fri, 27 Jan 2023 09:49:38 +0000 (UTC) Date: Fri, 27 Jan 2023 09:49:38 +0000 (UTC) From: Richard Biener To: Jakub Jelinek cc: Jeff Law , Andrew Pinski , Patrick Palka , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] tree: Fix up tree_code_{length,type} In-Reply-To: Message-ID: References: <1668794731-9349-1-git-send-email-apinski@marvell.com> User-Agent: Alpine 2.22 (LSU 394 2020-01-19) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,SPF_HELO_NONE,SPF_PASS,TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Fri, 27 Jan 2023, Jakub Jelinek wrote: > On Fri, Jan 27, 2023 at 07:42:39AM +0000, Richard Biener wrote: > > > BTW, wonder if tree_code_type couldn't be an array of unsigned char > > > elements rather than enum tree_code_class and we'd then cast it > > > to the enum in the macro, that would shrink that array from 1496 bytes > > > to 374. Of course, that sounds like stage1 material. > > > > One could argue the same way for this patch (and instead revert), > > Well, this patch is in fact a conditional reversion (revert for > C++11/14, add one keyword to 2 declarations otherwise). > > > I'd say if we tweak this now then tweak it to the maximum extent? > > Isn't sth like 'enum unsigned char tree_code_class' now possible? > > (and a static assert the enum values all fit, though that would > > be diagnosed anyway?) > > C++11 indeed has > enum tree_code_class : unsigned char { > tcc_exceptional, > ... > tcc_expression > }; > and one indeed gets an error if some enumerator doesn't fit. > The problem I see with this is that the type is 8-bit everywhere, > which I'd be afraid could cause worse code generation (of course, > one would need to try to see how much; e.g. build the compiler > unmodified, with the unsigned char array plus explicit casts from > the array and finally with unsigned char as underlying type). > When passing around enum tree_code_class etc., it is fine if it > is 32-bit. And there isn't a way to create an enum with different > underlying type but with the same enumerators as in another enum. > Perhaps for tree_code_class we could away with the underlying type > because it is mostly used in the macros which immediately compare > it, in gcc/*.cc just in the following explicitly: > expr.cc:get_def_for_expr_class (tree name, enum tree_code_class tclass) > fold-const.cc: enum tree_code_class tclass; > fold-const.cc: enum tree_code_class tclass = TREE_CODE_CLASS (code); > fold-const.cc: enum tree_code_class tclass = TREE_CODE_CLASS (code); > fold-const.cc: enum tree_code_class kind = TREE_CODE_CLASS (code); > fold-const.cc: enum tree_code_class kind = TREE_CODE_CLASS (code); > fold-const.cc: enum tree_code_class kind = TREE_CODE_CLASS (code); > fold-const.cc: enum tree_code_class kind = TREE_CODE_CLASS (code); > gimple-fold.cc: enum tree_code_class kind = TREE_CODE_CLASS (subcode); > print-tree.cc: enum tree_code_class tclass; > print-tree.cc: enum tree_code_class tclass; > tree.cc: These must correspond to the tree_code_class entries. */ > tree.cc:const char *const tree_code_class_strings[] = > tree.cc: enum tree_code_class type = TREE_CODE_CLASS (code); > tree.cc: enum tree_code_class type = TREE_CODE_CLASS (code); > tree.cc:tree_class_check_failed (const_tree node, const enum tree_code_class cl, > tree.cc:tree_not_class_check_failed (const_tree node, const enum tree_code_class cl, > tree.cc: const enum tree_code_class c = TREE_CODE_CLASS (TREE_CODE (t)); > tree.cc: const enum tree_code_class c = TREE_CODE_CLASS (TREE_CODE (t)); > tree-dump.cc: enum tree_code_class code_class; > tree-inline.cc: enum tree_code_class cl = TREE_CODE_CLASS (code); > tree-pretty-print.cc: enum tree_code_class tclass; > tree-ssa-live.cc: enum tree_code_class c = TREE_CODE_CLASS (TREE_CODE (t)); > tree-ssa-operands.cc: enum tree_code_class codeclass; > But as I said, one would need to watch for code generation at least on > a couple of common hosts, and while x86_64 should be one of them, it might > have bigger effects on others as x86 has byte comparison etc. instructions. Hm, yes. Not sure if using uint_fast8_t would make a difference where it should. So lets keep this change separate. Richard. > > > > > 2023-01-26 Patrick Palka > > > Jakub Jelinek > > > > > > * tree-core.h (tree_code_type, tree_code_length): For > > > C++17 and later, add inline keyword, otherwise don't define > > > the arrays, but declare extern arrays. > > > * tree.cc (tree_code_type, tree_code_length): Define these > > > arrays for C++14 and older. > > > > > > --- gcc/tree-core.h.jj 2023-01-02 09:32:31.188158094 +0100 > > > +++ gcc/tree-core.h 2023-01-26 16:02:34.212113251 +0100 > > > @@ -2284,17 +2284,20 @@ struct floatn_type_info { > > > /* Matrix describing the structures contained in a given tree code. */ > > > extern bool tree_contains_struct[MAX_TREE_CODES][64]; > > > > > > +/* Class of tree given its code. */ > > > +#if __cpp_inline_variables >= 201606L > > > #define DEFTREECODE(SYM, NAME, TYPE, LENGTH) TYPE, > > > #define END_OF_BASE_TREE_CODES tcc_exceptional, > > > > > > - > > > -/* Class of tree given its code. */ > > > -constexpr enum tree_code_class tree_code_type[] = { > > > +constexpr inline enum tree_code_class tree_code_type[] = { > > > #include "all-tree.def" > > > }; > > > > Do we need an explicit external definition somewhere when > > constant folding isn't possible? > > > > > Otherwise looks good to me. > > > > Thanks, > > Richard. > > > > > #undef DEFTREECODE > > > #undef END_OF_BASE_TREE_CODES > > > +#else > > > +extern const enum tree_code_class tree_code_type[]; > > There is one here for the C++11 and C++14 cases. > For C++17 and later it isn't needed, > constexpr inline enum tree_code_class tree_code_type[] = { > ... > }; > means this is a comdat variable in all TUs which need non-ODR > uses of it (tree_code_type[23] evaluates to constant expression, > but tree_code_type[x] or &tree_code_type[23] etc. often don't and then > the comdat var is emitted and all TUs share one copy of the variable. > > Jakub > > -- Richard Biener SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg, Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman; HRB 36809 (AG Nuernberg)