Re: enforce canonicalization of value_range's
On 8/14/19 1:53 PM, Jeff Law wrote:
On 8/13/19 6:51 PM, Aldy Hernandez wrote:
Presumably this was better than moving the implementation earlier.
Actually, it was for ease of review. I made some changes to the
function, and I didn't want the reviewer to miss them because I had
moved the function wholesale. I can move the function earlier, after we
agree on the changes (see below).
Either works for me. I think there was an informal effort to avoid
these kinds of forward decls eons ago because our inliner sucked, but in
the IPA world order in the source file really shouldn't matter.
Ok, I'll leave it as is, so I don't have to rebase the VARYING patch.
When both patches are in, I'll move the definition up as an obvious change.
If we weren't on a path to kill VRP I'd probably suggest a distinct
effort to constify this code. Some of the changes were a bit confusing
when it looked like we'd dropped a call to set the range of an object.
But those were just local copies, so setting the type/min/max directly
was actually fine. constification would make this a bit clearer. But
again, I don't think it's worth the effort given the long term
trajectory for tree-vrp.c.
I shouldn't be introducing any new confusion. Did I add any new methods
that should've been const that aren't? I can't see any??. I'm happy to
fix anything I introduced.
IIRC we had an incoming range object passed by value, which we locally
modified and called the setter.
I spotted the dropped call to the setter and was going to call it out as
possibly broken. But in investigating further I realized the object was
passed by value, so dropping the setter wasn't really a problem.
THe funny thing was we were doing this on source operands rather than
the destination operand. Arguably the ranges for the source operands
should be constant which would have flagged that code as fishy from its
inception and I'm sure the code would have been restructured
appropriately and would have avoided the confusion.
So in summary, you didn't break anything. It was a safe change you
made, but it wasn't immediately obvious it was safe. If we had a
constified codebase the intent of the code would have been more obvious.
So where does the handle_pointers stuff matter? I'm a bit surprised we
have to do anything special for them.
I've learned to touch as little of VRP as is necessary, as changing
anything to be more consistent breaks things in unexpected ways ;-).
In this particular case, TYPE_MIN_VALUE and TYPE_MAX_VALUE are not
defined for pointers, and I didn't want to change the meaning of
vrp_val_{min,max} throughout. I was trying to minimize the changes to
existing behavior. If it bothers you too much, we could remove it as a
follow up when we are sure there are no expected side-effects from the
rest of the patch. ??
I don't mind exploring this as a follow-up. I guess that a min/max
doesn't really have significant meaning for pointers.
I think rather than digging too deep into this, let's table it for now.
I think the time to revisit will be as we work through removal of
tree-vrp at some point in the future.
OK. I don't expect the answers to the minor questions above will
ultimately change anything.
I could appreciate a final nod before I commit. And even then, I will
wait until the other patch is approved and commit them simultaneously.
They are a bit intertwined.
I'm nodding :-)
I've tested this patch in isolation, and am committing it while we agree
on the varying one.
Thank you.
Aldy
Re: enforce canonicalization of value_range's
On 8/13/19 6:51 PM, Aldy Hernandez wrote:
>> Presumably this was better than moving the implementation earlier.
>
> Actually, it was for ease of review. I made some changes to the
> function, and I didn't want the reviewer to miss them because I had
> moved the function wholesale. I can move the function earlier, after we
> agree on the changes (see below).
Either works for me. I think there was an informal effort to avoid
these kinds of forward decls eons ago because our inliner sucked, but in
the IPA world order in the source file really shouldn't matter.
>
>>
>> If we weren't on a path to kill VRP I'd probably suggest a distinct
>> effort to constify this code. Some of the changes were a bit confusing
>> when it looked like we'd dropped a call to set the range of an object.
>> But those were just local copies, so setting the type/min/max directly
>> was actually fine. constification would make this a bit clearer. But
>> again, I don't think it's worth the effort given the long term
>> trajectory for tree-vrp.c.
>
> I shouldn't be introducing any new confusion. Did I add any new methods
> that should've been const that aren't? I can't see any??. I'm happy to
> fix anything I introduced.
IIRC we had an incoming range object passed by value, which we locally
modified and called the setter.
I spotted the dropped call to the setter and was going to call it out as
possibly broken. But in investigating further I realized the object was
passed by value, so dropping the setter wasn't really a problem.
THe funny thing was we were doing this on source operands rather than
the destination operand. Arguably the ranges for the source operands
should be constant which would have flagged that code as fishy from its
inception and I'm sure the code would have been restructured
appropriately and would have avoided the confusion.
So in summary, you didn't break anything. It was a safe change you
made, but it wasn't immediately obvious it was safe. If we had a
constified codebase the intent of the code would have been more obvious.
>
>>
>>
>> So where does the handle_pointers stuff matter? I'm a bit surprised we
>> have to do anything special for them.
>
> I've learned to touch as little of VRP as is necessary, as changing
> anything to be more consistent breaks things in unexpected ways ;-).
>
> In this particular case, TYPE_MIN_VALUE and TYPE_MAX_VALUE are not
> defined for pointers, and I didn't want to change the meaning of
> vrp_val_{min,max} throughout. I was trying to minimize the changes to
> existing behavior. If it bothers you too much, we could remove it as a
> follow up when we are sure there are no expected side-effects from the
> rest of the patch. ??
I don't mind exploring this as a follow-up. I guess that a min/max
doesn't really have significant meaning for pointers.
I think rather than digging too deep into this, let's table it for now.
I think the time to revisit will be as we work through removal of
tree-vrp at some point in the future.
>
>>
>>
>> OK. I don't expect the answers to the minor questions above will
>> ultimately change anything.
>
> I could appreciate a final nod before I commit. And even then, I will
> wait until the other patch is approved and commit them simultaneously.
> They are a bit intertwined.
I'm nodding :-)
jeff
Re: enforce canonicalization of value_range's
On 8/12/19 7:32 PM, Jeff Law wrote:
On 8/12/19 12:48 PM, Aldy Hernandez wrote:
This is a ping of:
https://gcc.gnu.org/ml/gcc-patches/2019-07/msg8.html
I have addressed the comments Jeff mentioned there.
This patch goes before the VR_VARYING + types patch, but I can adapt
either one to come first. I can even munge them together into one
patch, if it helps review.
Tested on x86-64 Linux.
OK (assuming ChangeLog entries)?
Aldy
curr.patch
commit ea908bdbfc8cdb4bb63e7d42630d01203edbac41
Author: Aldy Hernandez
Date: Mon Jul 15 18:09:27 2019 +0200
Enforce canonicalization in value_range.
Per your other message, I'll assume you'll write a suitable ChangeLog
entry before committing :-)
Attached patch has ChangeLog entries.
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 594ee9adc17..de2f39d8487 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -69,23 +69,20 @@ along with GCC; see the file COPYING3. If not see
#include "builtins.h"
#include "wide-int-range.h"
+static bool
+ranges_from_anti_range (const value_range_base *ar,
+ value_range_base *vr0, value_range_base *vr1,
+ bool handle_pointers = false);
+
Presumably this was better than moving the implementation earlier.
Actually, it was for ease of review. I made some changes to the
function, and I didn't want the reviewer to miss them because I had
moved the function wholesale. I can move the function earlier, after we
agree on the changes (see below).
+/* Normalize symbolics into constants. */
+
+value_range_base
+value_range_base::normalize_symbolics () const
+{
+ if (varying_p () || undefined_p ())
+return *this;
+ tree ttype = type ();
+ bool min_symbolic = !is_gimple_min_invariant (min ());
+ bool max_symbolic = !is_gimple_min_invariant (max ());
Sadly "symbolic" is a particularly poor term. When I think symbolics, I
think SYMBOL_REF, LABEL_REF and the tree equivalents. They're
*symbols*. Symbolics in this case are really things like SSA_NAMEs.
Confused the hell out of me briefly.
Sigh, yeah. Unfortunately, symbolics is what VRP calls anything that is
not an INTEGER_CST (that is, PLUS_EXPR and SSA_NAMEs, etc).
If we weren't on a path to kill VRP I'd probably suggest a distinct
effort to constify this code. Some of the changes were a bit confusing
when it looked like we'd dropped a call to set the range of an object.
But those were just local copies, so setting the type/min/max directly
was actually fine. constification would make this a bit clearer. But
again, I don't think it's worth the effort given the long term
trajectory for tree-vrp.c.
I shouldn't be introducing any new confusion. Did I add any new methods
that should've been const that aren't? I can't see any??. I'm happy to
fix anything I introduced.
So where does the handle_pointers stuff matter? I'm a bit surprised we
have to do anything special for them.
I've learned to touch as little of VRP as is necessary, as changing
anything to be more consistent breaks things in unexpected ways ;-).
In this particular case, TYPE_MIN_VALUE and TYPE_MAX_VALUE are not
defined for pointers, and I didn't want to change the meaning of
vrp_val_{min,max} throughout. I was trying to minimize the changes to
existing behavior. If it bothers you too much, we could remove it as a
follow up when we are sure there are no expected side-effects from the
rest of the patch. ??
OK. I don't expect the answers to the minor questions above will
ultimately change anything.
I could appreciate a final nod before I commit. And even then, I will
wait until the other patch is approved and commit them simultaneously.
They are a bit intertwined.
Thanks for your prompt reviews.
Aldy
commit 39be54a2e0ef19b3fb1cc3e51b380a6811e50ab4
Author: Aldy Hernandez
Date: Mon Jul 15 18:09:27 2019 +0200
Enforce canonicalization in value_range.
diff --git a/gcc/ChangeLog b/gcc/ChangeLog
index 93e600d68f2..3fa864624c9 100644
--- a/gcc/ChangeLog
+++ b/gcc/ChangeLog
@@ -1,3 +1,37 @@
+2019-08-13 Aldy Hernandez
+
+ * tree-vrp.c (value_range_base::set): Merge in code from
+ value_range_base::set_and_canonicalize.
+ Enforce canonicalization at set time.
+ Normalize [MIN, MAX] into VARYING and ~[MIN, MAX] into UNDEFINED.
+ (value_range_base::set_undefined): Inline call to set().
+ (value_range_base::set_varying): Same.
+ (value_range_base::singleton_p): Handle VR_ANTI_RANGEs.
+ (vrp_val_max): New argument handle_pointers.
+ (vrp_val_min): Same.
+ (ranges_from_anti_range): Same.
+ (extract_range_into_wide_ints): Use tree argument instead of sign
+ and precision.
+ (extract_range_from_multiplicative_op): Take in tree type instead
+ of precision and sign. Adapt function for canonicalized ranges.
+ (extract_range_from_binary_expr): Pass type to
+ extract_range_from_multiplicative_op.
+ Adapt for canonicalized ranges.
+ (extract_range_from_unary_expr): Same.
+
Re: enforce canonicalization of value_range's
On 8/12/19 12:48 PM, Aldy Hernandez wrote:
> This is a ping of:
>
> https://gcc.gnu.org/ml/gcc-patches/2019-07/msg8.html
>
> I have addressed the comments Jeff mentioned there.
>
> This patch goes before the VR_VARYING + types patch, but I can adapt
> either one to come first. I can even munge them together into one
> patch, if it helps review.
>
> Tested on x86-64 Linux.
>
> OK (assuming ChangeLog entries)?
>
> Aldy
>
>
> curr.patch
>
> commit ea908bdbfc8cdb4bb63e7d42630d01203edbac41
> Author: Aldy Hernandez
> Date: Mon Jul 15 18:09:27 2019 +0200
>
> Enforce canonicalization in value_range.
Per your other message, I'll assume you'll write a suitable ChangeLog
entry before committing :-)
>
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index 594ee9adc17..de2f39d8487 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -69,23 +69,20 @@ along with GCC; see the file COPYING3. If not see
> #include "builtins.h"
> #include "wide-int-range.h"
>
> +static bool
> +ranges_from_anti_range (const value_range_base *ar,
> + value_range_base *vr0, value_range_base *vr1,
> + bool handle_pointers = false);
> +
Presumably this was better than moving the implementation earlier.
> +/* Normalize symbolics into constants. */
> +
> +value_range_base
> +value_range_base::normalize_symbolics () const
> +{
> + if (varying_p () || undefined_p ())
> +return *this;
> + tree ttype = type ();
> + bool min_symbolic = !is_gimple_min_invariant (min ());
> + bool max_symbolic = !is_gimple_min_invariant (max ());
Sadly "symbolic" is a particularly poor term. When I think symbolics, I
think SYMBOL_REF, LABEL_REF and the tree equivalents. They're
*symbols*. Symbolics in this case are really things like SSA_NAMEs.
Confused the hell out of me briefly.
If we weren't on a path to kill VRP I'd probably suggest a distinct
effort to constify this code. Some of the changes were a bit confusing
when it looked like we'd dropped a call to set the range of an object.
But those were just local copies, so setting the type/min/max directly
was actually fine. constification would make this a bit clearer. But
again, I don't think it's worth the effort given the long term
trajectory for tree-vrp.c.
So where does the handle_pointers stuff matter? I'm a bit surprised we
have to do anything special for them.
OK. I don't expect the answers to the minor questions above will
ultimately change anything.
jeff
