From: Jeff Law <law@redhat.com>
To: Aldy Hernandez <aldyh@redhat.com>, gcc-patches <gcc-patches@gcc.gnu.org>
Cc: Andrew MacLeod <amacleod@redhat.com>
Subject: Re: types for VR_VARYING
Date: Wed, 14 Aug 2019 17:48:00 -0000 [thread overview]
Message-ID: <7357e66e-3ca6-dae1-5d22-067f5975677f@redhat.com> (raw)
In-Reply-To: <9586e8af-4552-9535-5bcd-a754fa1d7ccd@redhat.com>
On 8/13/19 6:39 PM, Aldy Hernandez wrote:
>
>
> On 8/12/19 7:46 PM, Jeff Law wrote:
>> On 8/12/19 12:43 PM, Aldy Hernandez wrote:
>>> This is a fresh re-post of:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2019-07/msg00006.html
>>>
>>> Andrew gave me some feedback a week ago, and I obviously don't remember
>>> what it was because I was about to leave on PTO.  However, I do remember
>>> I addressed his concerns before getting drunk on rum in tropical islands.
>>>
>> FWIW found a great coffee infused rum while in Kauai last week.  I'm not
>> a coffee fan, but it was wonderful.  The one bottle we brought back
>> isn't going to last until Cauldron and I don't think I can get a special
>> order filled before I leave :(
>
> You must bring some to Cauldron before we believe you. :)
That's the problem. The nearest place I can get it is in Vegas and
there's no distributor in Montreal. I can special order it in our
state run stores, but it won't be here in time.
Of course, I don't mind if you don't believe me. More for me in that
case...
>> Is the supports_type_p stuff there to placate the calls from ipa-cp?  I
>> can live with it in the short term, but it really feels like there
>> should be something in the ipa-cp client that avoids this silliness.
>
> I am not happy with this either, but there are various places where
> statements that are !stmt_interesting_for_vrp() are still setting a
> range of VARYING, which is then being ignored at a later time.
>
> For example, vrp_initialize:
>
>       if (!stmt_interesting_for_vrp (phi))
> Â Â Â Â Â Â Â Â {
>           tree lhs = PHI_RESULT (phi);
>           set_def_to_varying (lhs);
>           prop_set_simulate_again (phi, false);
> Â Â Â Â Â Â Â Â }
>
> Also in evrp_range_analyzer::record_ranges_from_stmt(), where we if the
> statement is interesting for VRP but extract_range_from_stmt() does not
> produce a useful range, we also set a varying for a range we will never
> use.  Similarly for a statement that is not interesting in this hunk.
Ugh. One could perhaps argue that setting any kind of range in these
circumstances is silly. But I suspect it's necessary due to the
optimistic handling of VR_UNDEFINED in value_range_base::union_helper.
It's all coming back to me now...
>
> Then there is vrp_prop::visit_stmt() where we also set VARYING for types
> that VRP will never handle:
>
>       case IFN_ADD_OVERFLOW:
>       case IFN_SUB_OVERFLOW:
>       case IFN_MUL_OVERFLOW:
>       case IFN_ATOMIC_COMPARE_EXCHANGE:
>     /* These internal calls return _Complex integer type,
>        which VRP does not track, but the immediate uses
>        thereof might be interesting.  */
>     if (lhs && TREE_CODE (lhs) == SSA_NAME)
> Â Â Â Â Â Â {
>         imm_use_iterator iter;
>         use_operand_p use_p;
>         enum ssa_prop_result res = SSA_PROP_VARYING;
>
>         set_def_to_varying (lhs);
>
> I've adjusted the patch so that set_def_to_varying will set the range to
> VR_UNDEFINED if !supports_type_p. This is a fail safe, as we can't
> really do anything with a nonsensical range. I just don't want to leave
> the range in an indeterminate state.
>
I think VR_UNDEFINED is unsafe due to value_range_base::union_helper.
And that's a more general than this patch. VR_UNDEFINED is _not_ a safe
range to set something to if we can't handle it. We have to use VR_VARYING.
Why? See the beginning of value_range_base::union_helper:
/* VR0 has the resulting range if VR1 is undefined or VR0 is varying. */
if (vr1->undefined_p ()
|| vr0->varying_p ())
return *vr0;
/* VR1 has the resulting range if VR0 is undefined or VR1 is varying. */
if (vr0->undefined_p ()
|| vr1->varying_p ())
return *vr1;
This can get called for something like
a = <cond> ? name1 : name2;
If name1 was set to VR_UNDEFINED thinking that VR_UNDEFINED was a safe
value for something we can't handle, then we'll incorrectly return the
range for name2.
VR_UNDEFINED can only be used for the ranges of objects we haven't
processed. If we can't produce a range for an object because the
statement is something we don't handle or just doesn't produce anythign
useful, then the right result is VR_VARYING.
This may be worth commenting at the definition site for VR_*.
>
> I also noticed that Andrew's patch was setting num_vr_values to
> num_ssa_names + num_ssa_names / 10. I think he meant num_vr_values +
> num_vr_values / 10.  Please verify the current incantation makes sense.
Going to assume this will be adjusted per the other messages in this thread.
> diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
> index 39ea22f0554..663dd6e2398 100644
> --- a/gcc/tree-ssa-threadedge.c
> +++ b/gcc/tree-ssa-threadedge.c
> @@ -182,8 +182,10 @@ record_temporary_equivalences_from_phis (edge e,
> new_vr->deep_copy (vr_values->get_value_range (src));
> else if (TREE_CODE (src) == INTEGER_CST)
> new_vr->set (src);
> + else if (value_range_base::supports_type_p (TREE_TYPE (src)))
> + new_vr->set_varying (TREE_TYPE (src));
> else
> - new_vr->set_varying ();
> + new_vr->set_undefined ();
So I think this can cause problems. VR_VARYING seems like the right
state here.
> +
> + if (!value_range_base::supports_type_p (TREE_TYPE (var)))
> + {
> + vr->set_undefined ();
> + return vr;
Probably better as VR_VARYING here too.
> + {
> + /* If we have an unsupported type (structs, void, etc), there
> + is nothing we'll be able to do with this entry.
> + Initialize it to UNDEFINED as a sanity measure, just in
> + case. */
> + vr->set_undefined ();
Here too.
Jeff
next prev parent reply other threads:[~2019-08-14 17:37 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-12 18:52 Aldy Hernandez
2019-08-13 0:35 ` Jeff Law
2019-08-14 4:23 ` Aldy Hernandez
2019-08-14 14:16 ` Andrew MacLeod
2019-08-14 14:39 ` Aldy Hernandez
2019-08-14 17:04 ` Jeff Law
2019-08-14 17:48 ` Jeff Law [this message]
2019-08-15 10:47 ` Aldy Hernandez
2019-08-15 11:28 ` Richard Biener
2019-08-15 13:10 ` Aldy Hernandez
2019-08-15 16:20 ` Aldy Hernandez
2019-08-16 8:47 ` Aldy Hernandez
2019-08-16 17:23 ` Jeff Law
2019-08-23 21:18 ` Martin Sebor
2019-08-24 21:15 ` Aldy Hernandez
2019-08-25 11:17 ` Martin Sebor
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=7357e66e-3ca6-dae1-5d22-067f5975677f@redhat.com \
--to=law@redhat.com \
--cc=aldyh@redhat.com \
--cc=amacleod@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
/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).