From: Richard Biener <richard.guenther@gmail.com>
To: "Martin Liška" <mliska@suse.cz>
Cc: Michael Matz <matz@suse.de>, GCC Patches <gcc-patches@gcc.gnu.org>
Subject: Re: [PATCH 5/9] Come up with an abstraction.
Date: Wed, 14 Aug 2019 14:38:00 -0000 [thread overview]
Message-ID: <CAFiYyc3Cm0fyjOQkfg9e7_cs0SSoC2mqqk4CDUuqtDzRZa3nDA@mail.gmail.com> (raw)
In-Reply-To: <f4481914-67c5-0c0d-ed4b-54d329b06af8@suse.cz>
On Wed, Aug 14, 2019 at 3:19 PM Martin Liška <mliska@suse.cz> wrote:
>
> On 8/14/19 3:04 PM, Richard Biener wrote:
> > On Mon, Aug 12, 2019 at 3:56 PM Martin Liška <mliska@suse.cz> wrote:
> >>
> >> On 8/12/19 2:43 PM, Richard Biener wrote:
> >>> On Mon, Aug 12, 2019 at 1:49 PM Martin Liška <mliska@suse.cz> wrote:
> >>>>
> >>>> On 8/12/19 1:40 PM, Richard Biener wrote:
> >>>>> On Mon, Aug 12, 2019 at 1:19 PM Martin Liška <mliska@suse.cz> 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 <mliska@suse.cz>
> >>>>>>>>
> >>>>>>>> * 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
_recursive calls_
> >
> > operand_equal_valueize (t1, t2, 0)
> > || operand_equal_p (t1, t2, 0)
> >
> > no?
>
> This is not going to work ..
I wonder if
class base
{
virtual operand_equal_p (tree a, tree b, int f);
};
base::operand_equal_p (tree a, tree b, int f)
{
as-is now, recursing to virtual operand_equal_p
}
class deriv : public base
{
vritual operand_equal_p (tree a, tree b, int f);
};
deriv::operand_equal_p (tree a, tree b, int f)
{
// just example
if (TREE_CODE (a) == TREE_CODE (b)
&& TREE_CODE (a) == FUNCTION_DECL)
return true;
return base::operand_equal_p (tree a, tree b, int f);
}
would work? ICF would call deriv::operand_equal_p and
base::operand_equal_p would recurse via the derived implementation.
That at least is cleaner from the "looks".
> > 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.
I wonder if all FUNCTION_DECL are really equal. If you just compare
the callgraph
you don't notice differences in the following (with disabled DSE/FRE
to retain both
stores to *dest)
void fna();
void fnb();
void foo (void *dest)
{
*dest = (void *)fna;
*dest = (void *)fnb;
}
void bar (void *dest)
{
*dest = (void *)fnb;
*dest = (void *)fna;
}
and if you compare IPA refs you'd need to identify the ref stmts as the same?
> 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.
> >>>>>>>
> >>>>>>
> >>>>
> >>
>
next prev parent reply other threads:[~2019-08-14 13:56 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-22 12:50 [RFC] operand_equal_p with valueization Jan Hubicka
2015-05-22 13:40 ` Richard Biener
2015-05-22 14:12 ` Jan Hubicka
2015-05-26 8:18 ` Richard Biener
2015-05-26 19:09 ` Jan Hubicka
2015-05-27 8:49 ` Richard Biener
2019-06-18 11:10 ` [RFC] " Martin Liška
2019-08-06 15:44 ` [PATCH 0/9] IPA ICF overhaul Martin Liska
2019-08-06 15:43 ` [PATCH 7/9] IPA ICF: remove dead code Martin Liska
2019-08-08 14:44 ` Jeff Law
2019-08-06 15:43 ` [PATCH 6/9] Integrate that for IPA ICF Martin Liska
2019-08-16 11:10 ` Martin Liška
2019-08-06 15:43 ` [PATCH 9/9] Remove alias set comparison Martin Liska
2019-08-07 15:58 ` Martin Sebor
2019-08-08 8:43 ` Martin Liška
2019-08-08 15:21 ` Martin Sebor
2019-08-08 14:44 ` Jeff Law
2019-08-06 15:43 ` [PATCH 5/9] Come up with an abstraction Martin Liska
2019-08-08 16:29 ` Michael Matz
2019-08-12 11:49 ` Martin Liška
2019-08-12 12:27 ` Richard Biener
2019-08-12 12:43 ` Martin Liška
2019-08-12 13:26 ` Richard Biener
2019-08-12 14:48 ` Martin Liška
2019-08-14 13:17 ` Richard Biener
2019-08-14 13:50 ` Martin Liška
2019-08-14 14:38 ` Richard Biener [this message]
2019-08-16 11:06 ` Martin Liška
2019-09-18 7:56 ` Martin Liška
2019-09-19 11:30 ` Richard Biener
2019-08-12 13:40 ` Michael Matz
2019-08-09 11:48 ` Richard Biener
2019-08-06 15:43 ` [PATCH 1/9] Replace int with boolean in predicate functions Martin Liska
2019-08-07 12:38 ` Richard Biener
2019-08-06 15:43 ` [PATCH 2/9] operand_equal_p: add support for FIELD_DECL Martin Liska
2019-08-07 12:21 ` Richard Biener
2019-08-15 14:19 ` Jan Hubicka
2019-08-16 9:28 ` Richard Biener
2019-08-16 12:17 ` Jan Hubicka
2019-09-11 12:58 ` Martin Liška
2019-08-06 15:43 ` [PATCH 8/9] Remove comparison for polymorphic types Martin Liska
2019-08-06 15:43 ` [PATCH 4/9] Strengthen alias_ptr_types_compatible_p in LTO mode Martin Liska
2019-08-07 12:05 ` Richard Biener
2019-08-08 12:09 ` Martin Liška
2019-08-09 11:20 ` Richard Biener
2019-08-06 15:55 ` [PATCH 3/9] operand_equal_p: add support for OBJ_TYPE_REF Martin Liska
2019-08-07 12:09 ` Richard Biener
2019-08-15 15:44 ` Jan Hubicka
2019-08-16 9:25 ` Richard Biener
2019-08-16 12:11 ` Jan Hubicka
2019-08-19 14:03 ` Richard Biener
2019-08-19 15:12 ` Jan Hubicka
2019-08-20 14:29 ` Richard Biener
2019-08-20 14:42 ` Jan Hubicka
2019-09-13 12:30 ` Martin Liška
2019-09-16 6:45 ` Richard Biener
2019-08-16 11:53 ` [PATCH 10/N] Use const_tree more in IPA ICF Martin Liška
2019-08-19 13:57 ` Richard Biener
2019-10-30 11:54 ` [PATCH 0/9] IPA ICF overhaul Martin Liška
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFiYyc3Cm0fyjOQkfg9e7_cs0SSoC2mqqk4CDUuqtDzRZa3nDA@mail.gmail.com \
--to=richard.guenther@gmail.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=matz@suse.de \
--cc=mliska@suse.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).