From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19040 invoked by alias); 14 Aug 2019 13:19: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 18618 invoked by uid 89); 14 Aug 2019 13:19:15 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-5.3 required=5.0 tests=AWL,BAYES_00,SPF_PASS autolearn=ham version=3.3.1 spammy=H*i:sk:CAFiYyc X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 14 Aug 2019 13:19:05 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id ADBA7B087; Wed, 14 Aug 2019 13:19:02 +0000 (UTC) Subject: Re: [PATCH 5/9] Come up with an abstraction. To: Richard Biener Cc: Michael Matz , GCC Patches References: <24e91baf28c01421ba05c877e00c058a2008a5cc.1565105736.git.mliska@suse.cz> <430c1435-a4c9-2960-5542-b95a8c346744@suse.cz> <11eca972-aa61-0ffa-04c1-5f42dbb9d32d@suse.cz> From: =?UTF-8?Q?Martin_Li=c5=a1ka?= Message-ID: Date: Wed, 14 Aug 2019 13:50:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2019-08/txt/msg00982.txt.bz2 On 8/14/19 3:04 PM, Richard Biener wrote: > On Mon, Aug 12, 2019 at 3:56 PM Martin Liška wrote: >> >> On 8/12/19 2:43 PM, Richard Biener wrote: >>> On Mon, Aug 12, 2019 at 1:49 PM Martin Liška wrote: >>>> >>>> On 8/12/19 1:40 PM, Richard Biener wrote: >>>>> On Mon, Aug 12, 2019 at 1:19 PM Martin Liška wrote: >>>>>> >>>>>> On 8/8/19 5:55 PM, Michael Matz wrote: >>>>>>> Hi, >>>>>>> >>>>>>> On Mon, 10 Jun 2019, Martin Liska wrote: >>>>>>> >>>>>>>> 2019-07-24 Martin Liska >>>>>>>> >>>>>>>> * fold-const.c (operand_equal_p): Rename to ... >>>>>>>> (operand_compare::operand_equal_p): ... this. >>>>>>>> (add_expr): Rename to ... >>>>>>>> (operand_compare::hash_operand): ... this. >>>>>>>> (operand_compare::operand_equal_valueize): Likewise. >>>>>>>> (operand_compare::hash_operand_valueize): Likewise. >>>>>>>> * fold-const.h (operand_equal_p): Set default >>>>>>>> value for last argument. >>>>>>>> (class operand_compare): New. >>>>>>> >>>>>>> Hmpf. A class without any data? That doesn't sound like a good design. >>>>>> >>>>>> Yes, the base class (current operand_equal_p) does not have a data. >>>>>> But the ICF derive class has a data and e.g. func_checker::operand_equal_valueize >>>>>> will use m_label_bb_map.get (t1). Which are member data of class func_checker. >>>>>> >>>>>>> You seem to need it only to have the possibility of virtual functions, >>>>>>> i.e. fancy callbacks. AFAICS you only have one derived class, i.e. a >>>>>>> simple distinction of two cases. What do you think about encoding the >>>>>>> additional new (ICF) case in the (existing) 'flags' argument to >>>>>>> operand_equal_p (and in case the ICF flag is set simply call the >>>>>>> "callback" directly)? >>>>>> >>>>>> That's possible. I can add two more callbacks to the operand_equal_p function >>>>>> (hash_operand_valueize and operand_equal_valueize). >>>>>> >>>>>> Is Richi also supporting this approach? >>>>> >>>>> I still see no value in the abstraction since you invoke none of the >>>>> (virtual) methods from the base class operand_equal_p. >>>> >>>> I call operand_equal_valueize (and hash_operand) from operand_equal_p. >>>> These are then used in IPA ICF (patch 6/9). >>> >>> Ugh. I see you call that after >>> >>> if (TREE_CODE (arg0) != TREE_CODE (arg1)) >>> { >>> ... >>> } >>> else >>> return false; >>> } >>> >>> and also after >>> >>> /* Check equality of integer constants before bailing out due to >>> precision differences. */ >>> if (TREE_CODE (arg0) == INTEGER_CST && TREE_CODE (arg1) == INTEGER_CST) >>> >>> which means for arg0 == SSA_NAME and arg1 == INTEGER_CST you return false >>> instead of valueizing arg0 to the possibly same or same "lose" value >>> and returning true. >> >> Yes. ICF does not allow to have anything where TREE_CODEs do not match. >> >>> >>> Also >>> >>> + int val = operand_equal_valueize (arg0, arg1, flags); >>> + if (val == 1) >>> + return 1; >>> + if (val == 0) >>> + return 0; >>> >>> suggests that you pass in arbirtrary trees for "valueization" but it >>> isn't actually >>> valueization that is performed but instead it should do an alternate comparison >>> of arg0 and arg1 with valueization. Why's this done this way instead of >>> sth like >>> >>> if (TREE_CODE (arg0) == SSA_NAME) >>> arg0 = operand_equal_valueize (arg0, flags); >>> if (TREE_CODE (arg1) == SSA_NAME) >>> arg1 = operand_equal_valueize (arg1, flags); >> >> Because I want to be given a pair of trees about which the function >> operand_equal_valueize returns match/no-match/dunno. >> >>> >>> and why's this done with virtual functions rather than a callback that we can >>> cheaply check for NULLness in the default implementation? >> >> I can transform it into a hook. But as mentioned I'll need two hooks. >> >>> >>> So - what does ICF want to make "equal" that isn't equal normally and how's >>> that "valueization"? >> >> E.g. for a FUNCTION_DECL, ICF always return true because it can only calls >> the operand_equal_p after callgraph is compared. Similarly for LABEL_DECLs, >> we have a map (m_label_bb_map). Please take a look at patch 6/9 in this >> series. > > Hmm, ok, so you basically replace recursive calls to operand_equal_p with > > operand_equal_valueize (t1, t2, 0) > || operand_equal_p (t1, t2, 0) > > no? This is not going to work .. > But the same could be achieved by actually making t1 and t2 equal > according to operand_equal_p rules via the valueization hook? So replace > FUNCTION_DECLs with their prevailing ones, LABEL_DECLs with theirs, etc. > > As given your abstraction is quite awkward to use, say, from value-numbering > which knows how to "valueize" a single tree but doesn't compare things. > > To make it work for your case you'd valueize not only SSA names but also > all DECL_P I guess. After all your operand_equal_valueize only does > something for "leafs" but is called for all intermediate expressions as well. ... because I need to be called for all intermediate expression. One simple example can be a ADDR_EXPR of a DECL. The first call will recursively call operand_equal_p for the DECL and the DECL can be compared with operand_equal_valueize in ICF. Note that current ICF code is more complex than only selection of a canonical form of a tree. I'm not saying the suggested API change is beautiful. But having a more specific equal hook seams to me a reasonable extension to current operand_equal_p. Moreover, we'll be able to kill all the ICF duplicate comparison machinery. Martin > > Richard. > >> Thanks, >> Martin >> >>> >>> Thanks, >>> Richard. >>> >>>> Martin >>>> >>>>> >>>>> Richard. >>>>> >>>>>> Thanks, >>>>>> Martin >>>>>> >>>>>>> IMHO that would also make the logic within >>>>>>> operand_equal_p clearer, because you don't have to think about all the >>>>>>> potential callback functions that might be called. >>>>>>> >>>>>>> >>>>>>> Ciao, >>>>>>> Michael. >>>>>>> >>>>>> >>>> >>