From: Jan Hubicka <hubicka@ucw.cz>
To: Michael Matz <matz@suse.de>
Cc: Jan Hubicka <hubicka@ucw.cz>, gcc-patches@gcc.gnu.org, rguenther@suse.de
Subject: Re: Do not compute alias sets for types that don't need them
Date: Tue, 26 May 2015 19:00:00 -0000 [thread overview]
Message-ID: <20150526173450.GD43680@kam.mff.cuni.cz> (raw)
In-Reply-To: <alpine.LSU.2.20.1505261545450.27315@wotan.suse.de>
> Hi,
>
> On Fri, 22 May 2015, Jan Hubicka wrote:
>
> > Index: tree-streamer-out.c
> > ===================================================================
> > --- tree-streamer-out.c (revision 223508)
> > +++ tree-streamer-out.c (working copy)
> > @@ -346,6 +346,7 @@ pack_ts_type_common_value_fields (struct
> > alias-set zero to this type. */
> > bp_pack_var_len_int (bp, (TYPE_ALIAS_SET (expr) == 0
> > || (!in_lto_p
> > + && type_with_alias_set_p (expr)
> > && get_alias_set (expr) == 0)) ? 0 : -1);
>
> I find such interfaces very ugly. IOW, when it's always (or often)
> necessary to call check_foo_p() before foo() can be called then the
> checking should be part of foo() (and it should then return a conservative
> value, i.e. alias set 0), and that requirement not be imposed on the
> callers of foo(). I.e. why can't whatever checks you do in
> type_with_alias_set_p be included in get_alias_set?
Because of sanity checking: I want to make alias sets of those types undefined
rather than having random values. The point is that using the alias set in
alias oracle querry is wrong.
Now I run into the case that we do produce MEM exprs for incomplete variants
just to take their address so I was thinking the other day about defining an
invalid alias set -2, making get_alias_set to return it and ICE later when query
is actually made?
We do have wrong query problems at least in ipa-icf, so I think it is worthwhile
sanity check.
>
> > + front-end routine) and use it.
> > +
> > + We may be called to produce MEM RTX for variable of incomplete type.
> > + This MEM RTX will only be used to produce address of a vairable, so
> > + we do not need to compute alias set. */
> > + if (!DECL_P (t) || type_with_alias_set_p (TYPE_MAIN_VARIANT (TREE_TYPE (t))))
> > + attrs.alias = get_alias_set (t);
>
> And if the checking needs to go down the main-variant chain then this
> should be done inside type_with_alias_set_p(), not in the caller,
> otherwise even the symmetry between arguments of type_with_alias_set_p(xy)
> and get_alias_set(xy) is destroyed (but see above for why I think
> type_with_alias_set_p shouldn't even exist).
Yep, good point - I will cleanup this.
Honza
next prev parent reply other threads:[~2015-05-26 17:34 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-05-22 12:33 Jan Hubicka
2015-05-22 13:32 ` Richard Biener
2015-05-22 13:42 ` Jan Hubicka
2015-05-26 8:16 ` Richard Biener
2015-05-27 6:33 ` Jan Hubicka
2015-05-27 7:59 ` Jan Hubicka
2015-05-27 8:50 ` Richard Biener
2015-05-26 14:07 ` Michael Matz
2015-05-26 19:00 ` Jan Hubicka [this message]
2015-05-27 8:39 ` Richard Biener
2015-05-27 16:24 ` Jan Hubicka
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=20150526173450.GD43680@kam.mff.cuni.cz \
--to=hubicka@ucw.cz \
--cc=gcc-patches@gcc.gnu.org \
--cc=matz@suse.de \
--cc=rguenther@suse.de \
/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).