[COMMITTED,4/4] - Implement pointer_or_operator.

Message ID 9ae81606-7e34-423c-b9d5-98147a43d5b6@redhat.com
State New
Headers
Series [COMMITTED,1/4] - Cleanup pointer_plus_operator. |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gcc_build--master-arm fail Patch failed to apply
linaro-tcwg-bot/tcwg_gcc_build--master-aarch64 fail Patch failed to apply

Commit Message

Andrew MacLeod Oct. 24, 2024, 12:53 p.m. UTC
  Well, perhaps the subject isn't precise  The existing 
pointer_or_operator is, like a few others, using irange operands, so is 
non-functional and this patch removes it.

The functionality was never moved to the new dispatch system when Prange 
was implemented, and IIRC it was because Aldy never found a case that 
triggered.  Regardless, this adds the functionality that once existed 
via operator_bitwise_or::fold_range() with Prange operands.

Bootstraps on x86_64-pc-linux-gnu with no regressions.  pushed.

Andrew
  

Comments

Mikael Morin Oct. 26, 2024, 7:08 p.m. UTC | #1
Hello,

Le 24/10/2024 à 14:53, Andrew MacLeod a écrit :
> diff --git a/gcc/range-op-ptr.cc b/gcc/range-op-ptr.cc
> index dd312a80366..ef2b2cce516 100644
> --- a/gcc/range-op-ptr.cc
> +++ b/gcc/range-op-ptr.cc
(...)
> -void
> -pointer_or_operator::wi_fold (irange &r, tree type,
> -			      const wide_int &lh_lb,
> -			      const wide_int &lh_ub,
> -			      const wide_int &rh_lb,
> -			      const wide_int &rh_ub) const
> +operator_bitwise_or::fold_range (prange &r, tree type,
> +				 const prange &op1,
> +				 const prange &op2,
> +				 relation_trio) const
>  {
>    // For pointer types, we are really only interested in asserting
>    // whether the expression evaluates to non-NULL.
> -  if (!wi_includes_zero_p (type, lh_lb, lh_ub)
> -      && !wi_includes_zero_p (type, rh_lb, rh_ub))
> +  if (!op1.zero_p () || !op2.zero_p ())
>      r.set_nonzero (type);

this doesn't feel right.  It checks that the operand range is anything 
but singleton [0], instead of checking that it does not contain 0.

> -  else if (wi_zero_p (type, lh_lb, lh_ub) && wi_zero_p (type, rh_lb, rh_ub))
> +  else if (op1.zero_p () && op2.zero_p ())
>      r.set_zero (type);
>    else
>      r.set_varying (type);

And it makes this else branch dead.

> +
> +  update_known_bitmask (r, BIT_IOR_EXPR, op1, op2);
> +  return true;
>  }
>  

For example, the change makes VARYING | 0 fold to non-zero (instead of 
VARYING).
  
Andrew MacLeod Oct. 28, 2024, 1:38 p.m. UTC | #2
On 10/26/24 15:08, Mikael Morin wrote:
> Hello,
>
> Le 24/10/2024 à 14:53, Andrew MacLeod a écrit :
>> diff --git a/gcc/range-op-ptr.cc b/gcc/range-op-ptr.cc
>> index dd312a80366..ef2b2cce516 100644
>> --- a/gcc/range-op-ptr.cc
>> +++ b/gcc/range-op-ptr.cc
> (...)
>> -void
>> -pointer_or_operator::wi_fold (irange &r, tree type,
>> -                  const wide_int &lh_lb,
>> -                  const wide_int &lh_ub,
>> -                  const wide_int &rh_lb,
>> -                  const wide_int &rh_ub) const
>> +operator_bitwise_or::fold_range (prange &r, tree type,
>> +                 const prange &op1,
>> +                 const prange &op2,
>> +                 relation_trio) const
>>  {
>>    // For pointer types, we are really only interested in asserting
>>    // whether the expression evaluates to non-NULL.
>> -  if (!wi_includes_zero_p (type, lh_lb, lh_ub)
>> -      && !wi_includes_zero_p (type, rh_lb, rh_ub))
>> +  if (!op1.zero_p () || !op2.zero_p ())
>>      r.set_nonzero (type);
>
> this doesn't feel right.  It checks that the operand range is anything 
> but singleton [0], instead of checking that it does not contain 0.


Hmm, i think you are right.  But I also think the original is wrong?  
Shouldn't it be nonzero if either operand doesn't contain zero?

It's Monday morning.. Check my logic please... :-)

Andrew

diff --git a/gcc/range-op-ptr.cc b/gcc/range-op-ptr.cc
index 9f500c52d2c..ccce05035b9 100644
--- a/gcc/range-op-ptr.cc
+++ b/gcc/range-op-ptr.cc
@@ -388,7 +388,7 @@ operator_bitwise_or::fold_range (prange &r, tree type,
  {
    // For pointer types, we are really only interested in asserting
    // whether the expression evaluates to non-NULL.
-  if (!op1.zero_p () || !op2.zero_p ())
+  if (!range_includes_zero_p (op1) || !range_includes_zero_p (op2))
      r.set_nonzero (type);
    else if (op1.zero_p () && op2.zero_p ())
      r.set_zero (type);

Andrew
  
Mikael Morin Oct. 28, 2024, 2:25 p.m. UTC | #3
Le 28/10/2024 à 14:38, Andrew MacLeod a écrit :
> 
> On 10/26/24 15:08, Mikael Morin wrote:
>> Hello,
>>
>> Le 24/10/2024 à 14:53, Andrew MacLeod a écrit :
>>> diff --git a/gcc/range-op-ptr.cc b/gcc/range-op-ptr.cc
>>> index dd312a80366..ef2b2cce516 100644
>>> --- a/gcc/range-op-ptr.cc
>>> +++ b/gcc/range-op-ptr.cc
>> (...)
>>> -void
>>> -pointer_or_operator::wi_fold (irange &r, tree type,
>>> -                  const wide_int &lh_lb,
>>> -                  const wide_int &lh_ub,
>>> -                  const wide_int &rh_lb,
>>> -                  const wide_int &rh_ub) const
>>> +operator_bitwise_or::fold_range (prange &r, tree type,
>>> +                 const prange &op1,
>>> +                 const prange &op2,
>>> +                 relation_trio) const
>>>  {
>>>    // For pointer types, we are really only interested in asserting
>>>    // whether the expression evaluates to non-NULL.
>>> -  if (!wi_includes_zero_p (type, lh_lb, lh_ub)
>>> -      && !wi_includes_zero_p (type, rh_lb, rh_ub))
>>> +  if (!op1.zero_p () || !op2.zero_p ())
>>>      r.set_nonzero (type);
>>
>> this doesn't feel right.  It checks that the operand range is anything 
>> but singleton [0], instead of checking that it does not contain 0.
> 
> 
> Hmm, i think you are right.  But I also think the original is wrong? 
> Shouldn't it be nonzero if either operand doesn't contain zero?
> 
> It's Monday morning.. Check my logic please... :-)
> 
The original wasn't strictly speaking wrong; I mean it was just 
producing more VARYING than necessary.
But otherwise, yes, your logic appears correct.

> 
> diff --git a/gcc/range-op-ptr.cc b/gcc/range-op-ptr.cc
> index 9f500c52d2c..ccce05035b9 100644
> --- a/gcc/range-op-ptr.cc
> +++ b/gcc/range-op-ptr.cc
> @@ -388,7 +388,7 @@ operator_bitwise_or::fold_range (prange &r, tree type,
>   {
>     // For pointer types, we are really only interested in asserting
>     // whether the expression evaluates to non-NULL.
> -  if (!op1.zero_p () || !op2.zero_p ())
> +  if (!range_includes_zero_p (op1) || !range_includes_zero_p (op2))
>       r.set_nonzero (type);
>     else if (op1.zero_p () && op2.zero_p ())
>       r.set_zero (type);
> 
And this looks good.
  

Patch

From 57f720af635e47d8b5515eddaef9df02769f353b Mon Sep 17 00:00:00 2001
From: Andrew MacLeod <amacleod@redhat.com>
Date: Wed, 23 Oct 2024 10:59:13 -0400
Subject: [PATCH 4/4] Implement pointer_or_operator.

The class pointer_or is no longer used, and can be removed.  Its
functionality was never moved to the new dispatch system.
This implements operator_bitwise_or::fold_range() for prange operands.

	* range-op-mixed.h (operator_bitwise_or::fold_range): Add prange
	variant.
	* range-op-ptr.cc (class pointer_or_operator): Remove.
	(pointer_or_operator::op1_range): Remove.
	(pointer_or_operator::op2_range): Remove.
	(pointer_or_operator::wi_fold): Remove.
	(operator_bitwise_or::fold_range): New prange variant.
---
 gcc/range-op-mixed.h |  6 +++++
 gcc/range-op-ptr.cc  | 63 +++++++-------------------------------------
 2 files changed, 16 insertions(+), 53 deletions(-)

diff --git a/gcc/range-op-mixed.h b/gcc/range-op-mixed.h
index cc1db2f6775..f7c447d935e 100644
--- a/gcc/range-op-mixed.h
+++ b/gcc/range-op-mixed.h
@@ -809,9 +809,15 @@  protected:
 class operator_bitwise_or : public range_operator
 {
 public:
+  using range_operator::fold_range;
   using range_operator::op1_range;
   using range_operator::op2_range;
   using range_operator::update_bitmask;
+
+  bool fold_range (prange &r, tree type,
+		   const prange &op1,
+		   const prange &op2,
+		   relation_trio) const final override;
   bool op1_range (irange &r, tree type,
 		  const irange &lhs, const irange &op2,
 		  relation_trio rel = TRIO_VARYING) const override;
diff --git a/gcc/range-op-ptr.cc b/gcc/range-op-ptr.cc
index dd312a80366..ef2b2cce516 100644
--- a/gcc/range-op-ptr.cc
+++ b/gcc/range-op-ptr.cc
@@ -379,69 +379,26 @@  pointer_plus_operator::op2_range (irange &r, tree type,
   return true;
 }
 
-
-class pointer_or_operator : public range_operator
-{
-public:
-  using range_operator::op1_range;
-  using range_operator::op2_range;
-  virtual bool op1_range (irange &r, tree type,
-			  const irange &lhs,
-			  const irange &op2,
-			  relation_trio rel = TRIO_VARYING) const;
-  virtual bool op2_range (irange &r, tree type,
-			  const irange &lhs,
-			  const irange &op1,
-			  relation_trio rel = TRIO_VARYING) const;
-  virtual void wi_fold (irange &r, tree type,
-			const wide_int &lh_lb, const wide_int &lh_ub,
-			const wide_int &rh_lb, const wide_int &rh_ub) const;
-} op_pointer_or;
-
-bool
-pointer_or_operator::op1_range (irange &r, tree type,
-				const irange &lhs,
-				const irange &op2 ATTRIBUTE_UNUSED,
-				relation_trio) const
-{
-  if (lhs.undefined_p ())
-    return false;
-  if (lhs.zero_p ())
-    {
-      r.set_zero (type);
-      return true;
-    }
-  r.set_varying (type);
-  return true;
-}
-
 bool
-pointer_or_operator::op2_range (irange &r, tree type,
-				const irange &lhs,
-				const irange &op1,
-				relation_trio) const
-{
-  return pointer_or_operator::op1_range (r, type, lhs, op1);
-}
-
-void
-pointer_or_operator::wi_fold (irange &r, tree type,
-			      const wide_int &lh_lb,
-			      const wide_int &lh_ub,
-			      const wide_int &rh_lb,
-			      const wide_int &rh_ub) const
+operator_bitwise_or::fold_range (prange &r, tree type,
+				 const prange &op1,
+				 const prange &op2,
+				 relation_trio) const
 {
   // For pointer types, we are really only interested in asserting
   // whether the expression evaluates to non-NULL.
-  if (!wi_includes_zero_p (type, lh_lb, lh_ub)
-      && !wi_includes_zero_p (type, rh_lb, rh_ub))
+  if (!op1.zero_p () || !op2.zero_p ())
     r.set_nonzero (type);
-  else if (wi_zero_p (type, lh_lb, lh_ub) && wi_zero_p (type, rh_lb, rh_ub))
+  else if (op1.zero_p () && op2.zero_p ())
     r.set_zero (type);
   else
     r.set_varying (type);
+
+  update_known_bitmask (r, BIT_IOR_EXPR, op1, op2);
+  return true;
 }
 
+
 class operator_pointer_diff : public range_operator
 {
   using range_operator::fold_range;
-- 
2.45.0