From: Andrew MacLeod <amacleod@redhat.com>
To: Jeff Law <law@redhat.com>, Richard Biener <richard.guenther@gmail.com>
Cc: Aldy Hernandez <aldyh@redhat.com>, gcc-patches <gcc-patches@gcc.gnu.org>
Subject: Re: [range-ops] patch 01/04: types for VR_UNDEFINED and VR_VARYING
Date: Wed, 24 Jul 2019 20:52:00 -0000 [thread overview]
Message-ID: <720577a0-adf6-71d2-3686-66831dcc5d59@redhat.com> (raw)
In-Reply-To: <828e67b6-d52c-f50d-27bb-46b3734a0493@redhat.com>
On 7/24/19 2:18 PM, Jeff Law wrote:
> On 7/24/19 11:00 AM, Richard Biener wrote:
> [ Big snip, ignore missing reply attributions... ]
>
>>> it. But I'd claim that if callers are required not to change these
>>> ranges, then the callers are fundamentally broken. I'm not sure
>>> what the "sanitization" is really buying you here. Can you point
>>> to something specific?
>>>
>>>> But you lose the sanitizing that nobody can change it and the
>>>> changed info leaks to other SSA vars.
>>>>
>>>> As said, fix all callers to deal with NULL.
>>>>
>>>> But I argue the current code is exactly optimal and safe.
>>> ANd I'd argue that it's just plain broken and that the
>>> sanitization you're referring to points to something broken
>>> elsewhere, higher up in the callers.
>> Another option is to make get_value_range return by value and the
>> only way to change the lattice to call an appropriate set function. I
>> think we already do the latter in all cases (but we use
>> get_value_range in the setter) and returning by reference is just
>> eliding the copy.
> OK, so what I think you're getting at (and please correct me if I'm
> wrong) is that once the lattice values are set, you don't want something
> changing the recorded ranges underneath?
>
> ISTM the way to enforce that is to embed the concept in the class and
> enforce it by not allowing direct manipulation of range by the clients.
> So a client that wants this behavior somehow tells the class that
> ranges are "set in stone" and from that point the setters don't allow
> changing the underlying ranges.
>
> I just want to make sure we're on the same page WRT why you think the
> constant varying range object is useful.
>
> jeff
That is not the functionality we are seeing.
whenever get_value_range silently returns a CONST varying node, the
ONLY way you can tell that the node might possibly be const elsewhere
would be if you first check that it is varying, like in :
void
vr_values::set_defs_to_varying (gimple *stmt)
{
 ssa_op_iter i;
 tree def;
 FOR_EACH_SSA_TREE_OPERAND (def, stmt, i, SSA_OP_DEF)
   {
     value_range *vr = get_value_range (def);
     /* Avoid writing to vr_const_varying get_value_range may
return. */
     if (!vr->varying_p ())
       vr->set_varying ();
   }
}
Which means there can be *no* context in which we ever try move one of
these nodes from varying to anything else, or we'd trap on a write to
read-only space.
Which means no place is ever trying to change those nodes from varying
to anything else. But nothing is preventing changes from other ranges
to something else.
Which also means the only thing this approach accomplishes is to force
us to check if a node is already varying, so that we don't overwrite the
node to varying just in case its a hidden const.
how can this hidden const node really be useful?
I submit this is just a dangerous way to flag previously unprocessed
nodes as VARYING for the duration of the pass after values_propagated is
set... not some higher level "Don't change this range any more" plan.Â
Its already bottom of the lattice.. it isn't going anywhere.
Andrew
next prev parent reply other threads:[~2019-07-24 19:50 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-01 8:52 Aldy Hernandez
2019-07-02 20:17 ` Jeff Law
2019-07-03 9:46 ` Aldy Hernandez
2019-07-03 22:40 ` Jeff Law
2019-07-03 8:28 ` Richard Biener
2019-07-03 9:19 ` Aldy Hernandez
2019-07-03 11:08 ` Richard Biener
2019-07-03 12:23 ` Aldy Hernandez
2019-07-03 12:30 ` Aldy Hernandez
2019-07-04 10:34 ` Richard Biener
2019-07-09 7:49 ` Aldy Hernandez
2019-07-09 9:57 ` Richard Biener
2019-07-16 19:56 ` Andrew MacLeod
2019-07-22 15:38 ` Aldy Hernandez
2019-07-23 0:19 ` Jeff Law
2019-07-23 9:45 ` Richard Biener
2019-07-24 16:09 ` Jeff Law
2019-07-24 17:06 ` Richard Biener
2019-07-24 18:33 ` Jeff Law
2019-07-24 18:38 ` Richard Biener
2019-07-26 3:41 ` Jeff Law
2019-07-26 14:52 ` Andrew MacLeod
2019-07-30 9:02 ` Richard Biener
2019-07-30 15:16 ` Andrew MacLeod
2019-07-31 8:37 ` Richard Biener
[not found] ` <78846d0a-354e-b73a-6e15-123752038fb2@redhat.com>
2019-08-01 14:11 ` Richard Biener
2019-08-01 16:35 ` Andrew MacLeod
2019-07-26 4:34 ` Jeff Law
2019-07-24 20:52 ` Andrew MacLeod [this message]
2019-07-25 9:13 ` Richard Biener
2019-07-23 9:34 ` Richard Biener
2019-07-23 23:05 ` Andrew MacLeod
2019-07-24 13:28 ` Richard Biener
2019-07-24 18:07 ` Andrew MacLeod
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=720577a0-adf6-71d2-3686-66831dcc5d59@redhat.com \
--to=amacleod@redhat.com \
--cc=aldyh@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
--cc=richard.guenther@gmail.com \
/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).