Re: types for VR_VARYING
On 8/24/19 4:55 AM, Aldy Hernandez wrote:
On 8/23/19 4:27 PM, Martin Sebor wrote:
On 8/15/19 10:06 AM, Aldy Hernandez wrote:
Hey Aldy,
After enabling EVRP for the strlen pass (as part of the sprintf
integration) I get a SEGV in the return statement in the function
below. Backing out this change gets rid of the ICE and lets my
tests pass, but I have no idea what the root cause of the SEGV
might be. The only mildly suspicious thing is the assertion in
the function:
@@ -355,9 +369,7 @@ value_range_base::singleton_p (tree *result) const
tree
value_range_base::type () const
{
- /* Types are only valid for VR_RANGE and VR_ANTI_RANGE, which are
- known to have non-zero min/max. */
- gcc_assert (min ());
+ gcc_assert (m_min || undefined_p ());
return TREE_TYPE (min ());
}
type() should really be:
tree
value_range_base::type () const
{
gcc_assert (m_min);
return TREE_TYPE (min ());
}
I should post a patch to fix this. However, UNDEF should never have a
type, so the assert will fail anyhow.
The code asking for the type of an UNDEF is wrong.
*this looks like so:
(gdb) p *this
$55 = {m_kind = VR_UNDEFINED, m_min = 0x0, m_max = 0x0}
so the assertion passes but the dererefence in TREE_TYPE fails.
The test case is trivial:
void g (const char *a, const char *b)
{
if (__builtin_memcmp (a, b, 8))
__builtin_abort ();
}
The ICE happens when updating the range for the second statement
below:
_1 = __builtin_memcmp (a_3(D), b_4(D), 8);
_1 = (int) _8;
Any ideas?
Martin
during GIMPLE pass: strlen
u.c: In function ‘g’:
u.c:1:6: internal compiler error: Segmentation fault
1 | void g (const char *a, const char *b)
| ^
0x11c4d08 crash_signal
/src/gcc/svn/gcc/toplev.c:326
0x815519 contains_struct_check(tree_node*, tree_node_structure_enum,
char const*, int, char const*)
/src/gcc/svn/gcc/tree.h:3376
0x15e9391 value_range_base::type() const
/src/gcc/svn/gcc/tree-vrp.c:373
0x16bdad6 vr_values::update_value_range(tree_node const*, value_range*)
/src/gcc/svn/gcc/vr-values.c:237
According to your backtrace, it looks like the call to type comes from
here:
/* Do not allow transitions up the lattice. The following
is slightly more awkward than just new_vr->type < old_vr->type
because VR_RANGE and VR_ANTI_RANGE need to be considered
the same. We may not have is_new when transitioning to
UNDEFINED. If old_vr->type is VARYING, we shouldn't be
called, if we are anyway, keep it VARYING. */
if (old_vr->varying_p ())
{
new_vr->set_varying (new_vr->type ());
is_new = false;
}
So new_vr is UNDEFINED and we're asking for it's type. That should
probably be:
new_vr->set_varying (TREE_TYPE (var));
Would you mind testing the attached patch?
If it passes, feel free to commit it as obvious.
Thanks for catching this.
Thanks for the quick fix! It cleared up the ICE for me.
Martin
Re: types for VR_VARYING
On 8/23/19 4:27 PM, Martin Sebor wrote:
On 8/15/19 10:06 AM, Aldy Hernandez wrote:
Hey Aldy,
After enabling EVRP for the strlen pass (as part of the sprintf
integration) I get a SEGV in the return statement in the function
below. Backing out this change gets rid of the ICE and lets my
tests pass, but I have no idea what the root cause of the SEGV
might be. The only mildly suspicious thing is the assertion in
the function:
@@ -355,9 +369,7 @@ value_range_base::singleton_p (tree *result) const
tree
value_range_base::type () const
{
- /* Types are only valid for VR_RANGE and VR_ANTI_RANGE, which are
- known to have non-zero min/max. */
- gcc_assert (min ());
+ gcc_assert (m_min || undefined_p ());
return TREE_TYPE (min ());
}
type() should really be:
tree
value_range_base::type () const
{
gcc_assert (m_min);
return TREE_TYPE (min ());
}
I should post a patch to fix this. However, UNDEF should never have a
type, so the assert will fail anyhow.
The code asking for the type of an UNDEF is wrong.
*this looks like so:
(gdb) p *this
$55 = {m_kind = VR_UNDEFINED, m_min = 0x0, m_max = 0x0}
so the assertion passes but the dererefence in TREE_TYPE fails.
The test case is trivial:
void g (const char *a, const char *b)
{
if (__builtin_memcmp (a, b, 8))
__builtin_abort ();
}
The ICE happens when updating the range for the second statement
below:
_1 = __builtin_memcmp (a_3(D), b_4(D), 8);
_1 = (int) _8;
Any ideas?
Martin
during GIMPLE pass: strlen
u.c: In function ‘g’:
u.c:1:6: internal compiler error: Segmentation fault
1 | void g (const char *a, const char *b)
| ^
0x11c4d08 crash_signal
/src/gcc/svn/gcc/toplev.c:326
0x815519 contains_struct_check(tree_node*, tree_node_structure_enum,
char const*, int, char const*)
/src/gcc/svn/gcc/tree.h:3376
0x15e9391 value_range_base::type() const
/src/gcc/svn/gcc/tree-vrp.c:373
0x16bdad6 vr_values::update_value_range(tree_node const*, value_range*)
/src/gcc/svn/gcc/vr-values.c:237
According to your backtrace, it looks like the call to type comes from here:
/* Do not allow transitions up the lattice. The following
is slightly more awkward than just new_vr->type < old_vr->type
because VR_RANGE and VR_ANTI_RANGE need to be considered
the same. We may not have is_new when transitioning to
UNDEFINED. If old_vr->type is VARYING, we shouldn't be
called, if we are anyway, keep it VARYING. */
if (old_vr->varying_p ())
{
new_vr->set_varying (new_vr->type ());
is_new = false;
}
So new_vr is UNDEFINED and we're asking for it's type. That should
probably be:
new_vr->set_varying (TREE_TYPE (var));
Would you mind testing the attached patch?
If it passes, feel free to commit it as obvious.
Thanks for catching this.
Aldy
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 8067f8560cd..97a82e6ee24 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -369,7 +369,7 @@ value_range_base::singleton_p (tree *result) const
tree
value_range_base::type () const
{
- gcc_assert (m_min || undefined_p ());
+ gcc_assert (m_min);
return TREE_TYPE (min ());
}
diff --git a/gcc/vr-values.c b/gcc/vr-values.c
index 6f9a3612931..96c764c987b 100644
--- a/gcc/vr-values.c
+++ b/gcc/vr-values.c
@@ -234,7 +234,7 @@ vr_values::update_value_range (const_tree var, value_range *new_vr)
called, if we are anyway, keep it VARYING. */
if (old_vr->varying_p ())
{
- new_vr->set_varying (new_vr->type ());
+ new_vr->set_varying (TREE_TYPE (var));
is_new = false;
}
else if (new_vr->undefined_p ())
Re: types for VR_VARYING
On 8/15/19 10:06 AM, Aldy Hernandez wrote:
Hey Aldy,
After enabling EVRP for the strlen pass (as part of the sprintf
integration) I get a SEGV in the return statement in the function
below. Backing out this change gets rid of the ICE and lets my
tests pass, but I have no idea what the root cause of the SEGV
might be. The only mildly suspicious thing is the assertion in
the function:
@@ -355,9 +369,7 @@ value_range_base::singleton_p (tree *result) const
tree
value_range_base::type () const
{
- /* Types are only valid for VR_RANGE and VR_ANTI_RANGE, which are
- known to have non-zero min/max. */
- gcc_assert (min ());
+ gcc_assert (m_min || undefined_p ());
return TREE_TYPE (min ());
}
*this looks like so:
(gdb) p *this
$55 = {m_kind = VR_UNDEFINED, m_min = 0x0, m_max = 0x0}
so the assertion passes but the dererefence in TREE_TYPE fails.
The test case is trivial:
void g (const char *a, const char *b)
{
if (__builtin_memcmp (a, b, 8))
__builtin_abort ();
}
The ICE happens when updating the range for the second statement
below:
_1 = __builtin_memcmp (a_3(D), b_4(D), 8);
_1 = (int) _8;
Any ideas?
Martin
during GIMPLE pass: strlen
u.c: In function ‘g’:
u.c:1:6: internal compiler error: Segmentation fault
1 | void g (const char *a, const char *b)
| ^
0x11c4d08 crash_signal
/src/gcc/svn/gcc/toplev.c:326
0x815519 contains_struct_check(tree_node*, tree_node_structure_enum,
char const*, int, char const*)
/src/gcc/svn/gcc/tree.h:3376
0x15e9391 value_range_base::type() const
/src/gcc/svn/gcc/tree-vrp.c:373
0x16bdad6 vr_values::update_value_range(tree_node const*, value_range*)
/src/gcc/svn/gcc/vr-values.c:237
0x200f9a9 evrp_range_analyzer::record_ranges_from_stmt(gimple*, bool)
/src/gcc/svn/gcc/gimple-ssa-evrp-analyze.c:325
0x14c9062 strlen_dom_walker::before_dom_children(basic_block_def*)
/src/gcc/svn/gcc/tree-ssa-strlen.c:4800
0x1fbdf8a dom_walker::walk(basic_block_def*)
/src/gcc/svn/gcc/domwalk.c:309
0x14c9362 printf_strlen_execute
/src/gcc/svn/gcc/tree-ssa-strlen.c:4866
0x14c95f8 execute
/src/gcc/svn/gcc/tree-ssa-strlen.c:4968
Please submit a full bug report,
Re: types for VR_VARYING
On 8/15/19 10:06 AM, Aldy Hernandez wrote:
>
>
> On 8/15/19 7:23 AM, Richard Biener wrote:
>> On Thu, Aug 15, 2019 at 12:40 PM Aldy Hernandez wrote:
>>>
>>> On 8/14/19 1:37 PM, Jeff Law wrote:
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/msg6.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 = ? 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.
>>>
>>> I think that if name1 was !supports_type_p, we will have never called
>>> union/intersect. We will have bailed at some point earlier. However, I
>>> do see your point about being consistent.
>>>
VR_UNDEFINED can only be used for the ranges of objects we haven't
>>
Re: types for VR_VARYING
On 8/15/19 12:06 PM, Aldy Hernandez wrote:
On 8/15/19 7:23 AM, Richard Biener wrote:
On Thu, Aug 15, 2019 at 12:40 PM Aldy Hernandez wrote:
On 8/14/19 1:37 PM, Jeff Law wrote:
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/msg6.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 = ? 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.
I think that if name1 was !supports_type_p, we will have never called
union/intersect. We will have bailed at some point earlier. However, I
do see your point about being consistent.
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.
Done.
diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index 39ea22f0554..663dd6e2398 100644
--- a/gcc/tre
Re: types for VR_VARYING
On 8/15/19 7:23 AM, Richard Biener wrote:
On Thu, Aug 15, 2019 at 12:40 PM Aldy Hernandez wrote:
On 8/14/19 1:37 PM, Jeff Law wrote:
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/msg6.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 = ? 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.
I think that if name1 was !supports_type_p, we will have never called
union/intersect. We will have bailed at some point earlier. However, I
do see your point about being consistent.
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.
Done.
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 @
Re: types for VR_VARYING
On 8/15/19 7:23 AM, Richard Biener wrote:
On Thu, Aug 15, 2019 at 12:40 PM Aldy Hernandez wrote:
On 8/14/19 1:37 PM, Jeff Law wrote:
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/msg6.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 = ? 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.
I think that if name1 was !supports_type_p, we will have never called
union/intersect. We will have bailed at some point earlier. However, I
do see your point about being consistent.
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.
Done.
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
Re: types for VR_VARYING
On Thu, Aug 15, 2019 at 12:40 PM Aldy Hernandez wrote:
>
> On 8/14/19 1:37 PM, Jeff Law wrote:
> > 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/msg6.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 = ? 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.
>
> I think that if name1 was !supports_type_p, we will have never called
> union/intersect. We will have bailed at some point earlier. However, I
> do see your point about being consistent.
>
> >
> > 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_
Re: types for VR_VARYING
On 8/14/19 1:37 PM, Jeff Law wrote:
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/msg6.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 = ? 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.
I think that if name1 was !supports_type_p, we will have never called
union/intersect. We will have bailed at some point earlier. However, I
do see your point about being consistent.
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.
Done.
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));
e
Re: types for VR_VARYING
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/msg6.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 = ? 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_
Re: types for VR_VARYING
On 8/14/19 8:15 AM, Aldy Hernandez wrote: > > > On 8/14/19 9:50 AM, Andrew MacLeod wrote: >> On 8/13/19 8:39 PM, Aldy Hernandez wrote: >>> >>> >>> Yes, it was 2X. >>> >>> I noticed that Richi made some changes to the lattice handling for >>> VARYING while the discussion was on-going. I missed these, and had >>> failed to adapt the patch for it. I would appreciate a final review >>> of the attached patch, especially the vr-values.c changes, which I >>> have modified to play nice with current trunk. >>> >>> 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. >>> >> no, I meant num_ssa_names. We are resizing the vector because >> num_vr_values is out of date (and smaller than num_ssa_names is now), >> so we need to resize the vector to be at least the number of >> ssa-names... and I added 10% just in case we arent done adding new ones. >> >> >> if num_vr_values is 100, and we've added 200 ssa-names, num_ssa_names >> would now be 300. if you resize based on num_vr_values, you could >> still go off the end of the vector. > > OK, I've changed the resize to allocate 2X as well. So now we'll have: > > + unsigned int old_sz = num_vr_values; > + num_vr_values = num_ssa_names * 2; > + vr_value = XRESIZEVEC (value_range *, vr_value, num_vr_values); > etc > > And the original allocation will also be 2X. I don't think we want the resize to be 2X, we've tried to get away from those kinds of growth patterns. The 10% from Andrew's patch seems like a better choice for the resize. jeff
Re: types for VR_VARYING
On 8/14/19 9:50 AM, Andrew MacLeod wrote: On 8/13/19 8:39 PM, Aldy Hernandez wrote: Yes, it was 2X. I noticed that Richi made some changes to the lattice handling for VARYING while the discussion was on-going. I missed these, and had failed to adapt the patch for it. I would appreciate a final review of the attached patch, especially the vr-values.c changes, which I have modified to play nice with current trunk. 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. no, I meant num_ssa_names. We are resizing the vector because num_vr_values is out of date (and smaller than num_ssa_names is now), so we need to resize the vector to be at least the number of ssa-names... and I added 10% just in case we arent done adding new ones. if num_vr_values is 100, and we've added 200 ssa-names, num_ssa_names would now be 300. if you resize based on num_vr_values, you could still go off the end of the vector. OK, I've changed the resize to allocate 2X as well. So now we'll have: + unsigned int old_sz = num_vr_values; + num_vr_values = num_ssa_names * 2; + vr_value = XRESIZEVEC (value_range *, vr_value, num_vr_values); etc And the original allocation will also be 2X. Aldy
Re: types for VR_VARYING
On 8/13/19 8:39 PM, Aldy Hernandez wrote: Yes, it was 2X. I noticed that Richi made some changes to the lattice handling for VARYING while the discussion was on-going. I missed these, and had failed to adapt the patch for it. I would appreciate a final review of the attached patch, especially the vr-values.c changes, which I have modified to play nice with current trunk. 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. no, I meant num_ssa_names. We are resizing the vector because num_vr_values is out of date (and smaller than num_ssa_names is now), so we need to resize the vector to be at least the number of ssa-names... and I added 10% just in case we arent done adding new ones. if num_vr_values is 100, and we've added 200 ssa-names, num_ssa_names would now be 300. if you resize based on num_vr_values, you could still go off the end of the vector. Andrew
Re: types for VR_VARYING
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/msg6.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. :)
Their more traditional rums were good as well. Koloa Rum. See if you
can get it locally :-)
This patch adds MIN/MAX values for VR_VARYING. As was discussed
earlier, we are only changing VR_VARYING, not VR_UNDEFINED as was the
original plan.
As I mentioned earlier, I am tired of re-doing ChangeLogs, so I'll whip
up the changelog when the review starts.
ACK.
ChangeLog entries included in attached patch.
@@ -150,12 +166,11 @@ 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 ();
-}
+if (value_range_base::supports_type_p (TREE_TYPE (def)))
+ {
+ value_range *vr = get_value_range (def);
+ vr->set_varying (TREE_TYPE (def));
+ }
}
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.
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.
/* Update the value range and equivalence set for variable VAR to
@@ -1920,7 +1935,7 @@ vr_values::dump_all_value_ranges (FILE *file)
vr_values::vr_values () : vrp_value_range_pool ("Tree VRP value ranges")
{
values_propagated = false;
- num_vr_values = num_ssa_names;
+ num_vr_values = num_ssa_names + num_ssa_names / 10;
vr_value = XCNEWVEC (value_range *, num_vr_values);
vr_phi_edge_counts = XCNEWVEC (int, num_ssa_names);
bitmap_obstack_initialize (&vrp_equiv_obstack);
My recollection was someone (Richi) proposed 2X for the initial
allocation which should ensure that in all but the most pathological
cases that we never trigger a reallocation. In terms of "wasted" space,
it shouldn't matter in practice.
So if you could check the old thread and verify that Richi recommended
the 2X initial allocation and if so, make that minor update.
Yes, it was 2X.
I noticed that Richi made some changes to the lattice handling for
VARYING while the discussion was on-going. I missed these, and had
failed to adapt the patch for it. I would appreciate a final review of
the attached patch, especially the vr-values.c changes, which I have
modified to play nice with current trunk.
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.
Tested on x86-64 Linux.
OK for trunk?
Aldy
commit 4c21b0f667176933f67469372da7459b8cf0e
Re: types for VR_VARYING
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/msg6.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 :(
Their more traditional rums were good as well. Koloa Rum. See if you
can get it locally :-)
>
> This patch adds MIN/MAX values for VR_VARYING. As was discussed
> earlier, we are only changing VR_VARYING, not VR_UNDEFINED as was the
> original plan.
>
> As I mentioned earlier, I am tired of re-doing ChangeLogs, so I'll whip
> up the changelog when the review starts.
ACK.
> @@ -150,12 +166,11 @@ 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 ();
> -}
> +if (value_range_base::supports_type_p (TREE_TYPE (def)))
> + {
> + value_range *vr = get_value_range (def);
> + vr->set_varying (TREE_TYPE (def));
> + }
> }
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.
>
> /* Update the value range and equivalence set for variable VAR to
> @@ -1920,7 +1935,7 @@ vr_values::dump_all_value_ranges (FILE *file)
> vr_values::vr_values () : vrp_value_range_pool ("Tree VRP value ranges")
> {
>values_propagated = false;
> - num_vr_values = num_ssa_names;
> + num_vr_values = num_ssa_names + num_ssa_names / 10;
>vr_value = XCNEWVEC (value_range *, num_vr_values);
>vr_phi_edge_counts = XCNEWVEC (int, num_ssa_names);
>bitmap_obstack_initialize (&vrp_equiv_obstack);
My recollection was someone (Richi) proposed 2X for the initial
allocation which should ensure that in all but the most pathological
cases that we never trigger a reallocation. In terms of "wasted" space,
it shouldn't matter in practice.
So if you could check the old thread and verify that Richi recommended
the 2X initial allocation and if so, make that minor update.
Jeff
types for VR_VARYING
This is a fresh re-post of:
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg6.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.
This patch adds MIN/MAX values for VR_VARYING. As was discussed
earlier, we are only changing VR_VARYING, not VR_UNDEFINED as was the
original plan.
As I mentioned earlier, I am tired of re-doing ChangeLogs, so I'll whip
up the changelog when the review starts.
Tested on x86-64 Linux.
OK?
Aldy
commit dad943a48ff2fccc203bf11839b7e3016e44dfe1
Author: Aldy Hernandez
Date: Mon Jul 22 10:04:43 2019 +0200
Add type to VR_VARYING.
diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
index 4c68af847e1..0184703a13c 100644
--- a/gcc/gimple-ssa-evrp-analyze.c
+++ b/gcc/gimple-ssa-evrp-analyze.c
@@ -251,13 +251,18 @@ evrp_range_analyzer::record_ranges_from_phis (basic_block bb)
if (virtual_operand_p (lhs))
continue;
+ /* Skips floats and other things we can't represent in a
+ range. */
+ if (!value_range_base::supports_type_p (TREE_TYPE (lhs)))
+ continue;
+
value_range vr_result;
bool interesting = stmt_interesting_for_vrp (phi);
if (!has_unvisited_preds && interesting)
vr_values->extract_range_from_phi_node (phi, &vr_result);
else
{
- vr_result.set_varying ();
+ vr_result.set_varying (TREE_TYPE (lhs));
/* When we have an unvisited executable predecessor we can't
use PHI arg ranges which may be still UNDEFINED but have
to use VARYING for them. But we can still resort to
diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
index 69c00a9c5a5..859ad3fbaf5 100644
--- a/gcc/ipa-cp.c
+++ b/gcc/ipa-cp.c
@@ -977,7 +977,12 @@ ipcp_vr_lattice::set_to_bottom ()
{
if (m_vr.varying_p ())
return false;
- m_vr.set_varying ();
+ /* ?? We create all sorts of VARYING ranges for floats, structures,
+ and other types which we cannot handle as ranges. We should
+ probably avoid handling them throughout the pass, but it's easier
+ to create a sensible VARYING here and let the lattice
+ propagate. */
+ m_vr.set_varying (integer_type_node);
return true;
}
diff --git a/gcc/tree-ssa-threadedge.c b/gcc/tree-ssa-threadedge.c
index 785227df690..43bca63da09 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 ();
/* This is a temporary range for DST, so push it. */
evrp_range_analyzer->push_value_range (dst, new_vr);
diff --git a/gcc/tree-ssanames.c b/gcc/tree-ssanames.c
index 8b80bce8945..3911db9c26e 100644
--- a/gcc/tree-ssanames.c
+++ b/gcc/tree-ssanames.c
@@ -440,14 +440,16 @@ get_range_info (const_tree name, value_range_base &vr)
wide_int wmin, wmax;
enum value_range_kind kind = get_range_info (name, &wmin, &wmax);
- if (kind == VR_VARYING || kind == VR_UNDEFINED)
-min = max = NULL;
+ if (kind == VR_VARYING)
+vr.set_varying (TREE_TYPE (name));
+ else if (kind == VR_UNDEFINED)
+vr.set_undefined ();
else
{
min = wide_int_to_tree (TREE_TYPE (name), wmin);
max = wide_int_to_tree (TREE_TYPE (name), wmax);
+ vr.set (kind, min, max);
}
- vr.set (kind, min, max);
return kind;
}
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index de2f39d8487..e215b032f78 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -186,9 +186,11 @@ value_range_base::check ()
break;
}
case VR_UNDEFINED:
-case VR_VARYING:
gcc_assert (!min () && !max ());
break;
+case VR_VARYING:
+ gcc_assert (m_min && m_max);
+ break;
default:
gcc_unreachable ();
}
@@ -214,6 +216,10 @@ value_range::check ()
bool
value_range_base::equal_p (const value_range_base &other) const
{
+ /* Ignore types for undefined. All undefines are equal. */
+ if (undefined_p ())
+return m_kind == other.m_kind;
+
return (m_kind == other.m_kind
&& vrp_operand_equal_p (m_min, other.m_min)
&& vrp_operand_equal_p (m_max, other.m_max));
@@ -269,16 +275,19 @@ value_range::set_undefined ()
}
void
-value_range_base::set_varying ()
+value_range_base::set_varying (tree type)
{
+ gcc_assert (supports_type_p (type));
m_kind = VR_VARYING;
- m_min = m_max = NULL;
+ m_min = vrp_val_min (type, true);
+ m_max = vrp_val_max (type, true);
}
void
-value_range::set_varying ()
+value_range::set_varying (tree type)
{
- set (VR_VARYING, NULL, NULL, NULL);
+ value_range_base::set_varying (type);
+ equiv_clear ();
}
/* Return TRUE
