Re: [PATCH 1/2]middle-end: Fix wrong overmatching of div-bitmask by using new optabs [PR108583]

[Andrew MacLeod <amacleod@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5050 bytes --]


On 2/22/23 08:06, Tamar Christina wrote:
> Hi Andrew,
>
>> all the range-op integer code is in gcc/range-op.cc.  As this is a basic
>> binary operation, you should be able to get away with implementing a
>> single routine,  wi_fold () which adds 2 wide int bounds  together and
>> returns a result.  THis si the implelemntaion for operator_plus.
>>
>> void
>> operator_plus::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
>> {
>>     wi::overflow_type ov_lb, ov_ub;
>>     signop s = TYPE_SIGN (type);
>>     wide_int new_lb = wi::add (lh_lb, rh_lb, s, &ov_lb);
>>     wide_int new_ub = wi::add (lh_ub, rh_ub, s, &ov_ub);
>>     value_range_with_overflow (r, type, new_lb, new_ub, ov_lb, ov_ub);
>> }
>>
>>
>> you shouldn't have to do any of the overflow stuff at the end, just take
>> the 2 sets of wide int, double their precision to start, add them
>> together (it cant possible overflow right) and then return an
>> int_range<2> with those bounds...
>> ie
>>
>> void
>> operator_plus::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
>> {
>>     wi::overflow_type ov_lb, ov_ub;
>>     signop s = TYPE_SIGN (type);
>>
>>     // Do whatever wideint magic is required to do this adds in higher
>> precision
>>     wide_int new_lb = wi::add (lh_lb, rh_lb, s, &ov_lb);
>>     wide_int new_ub = wi::add (lh_ub, rh_ub, s, &ov_ub);
>>
>>     r = int_range<2> (type, new_lb, new_ub);
>> }
>>
> So I've been working on adding support for widening plus and widening multiplication,
> and my examples all work now.. but during bootstrap I hit a problem.
>
> Say you have a mixed sign widening multiplication, such as in:
>
> int decMultiplyOp_zacc, decMultiplyOp_iacc;
> int *decMultiplyOp_lp;
> void decMultiplyOp() {
>    decMultiplyOp_lp = &decMultiplyOp_zacc;
>    for (; decMultiplyOp_lp < &decMultiplyOp_zacc + decMultiplyOp_iacc;
>         decMultiplyOp_lp++)
>      *decMultiplyOp_lp = 0;
> }
>
> Eventually the pointer arithmetic will generate:
>
> intD.7 decMultiplyOp_iacc.2_13;
> long unsigned intD.11 _15;
> _15 = decMultiplyOp_iacc.2_13 w* 4;
> and it'll try to get the range from this.
>
> My implementation is just:
>
> void
> operator_widen_mult::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
> {
>    signop s = TYPE_SIGN (type);
>
>    wide_int lh_wlb = wide_int::from (lh_lb, wi::get_precision (lh_lb) * 2, s);
>    wide_int rh_wlb = wide_int::from (rh_lb, wi::get_precision (rh_lb) * 2, s);
>    wide_int lh_wub = wide_int::from (lh_ub, wi::get_precision (lh_ub) * 2, s);
>    wide_int rh_wub = wide_int::from (rh_ub, wi::get_precision (rh_ub) * 2, s);
>
>    /* We don't expect a widening multiplication to be able to overflow but range
>       calculations for multiplications are complicated.  After widening the
>       operands lets call the base class.  */
>    return operator_mult::wi_fold (r, type, lh_wlb, lh_wub, rh_wlb, rh_wub);
> }
>
> But in this case the operands are different types and the wi_fold only gets the
> type of the operation. The issue is that when increasing the precision for lh_*
> I need to sign extend the value and not zero extend, but I don't seem to have
> enough context here to know that I do.  I'm missing the type of the operands.
>
> For non-widening operations this doesn't matter as the precision stays the same.
>
> Is there a way to get the information I need?
>
>
we haven't had this situation before, if I understand it correctly:

The LHS is a different type than both the operands, and your problem is 
you need to know the sign of at least operand1 in order to know whether 
to zero extend or to sign extend it?  huh. haven't run into that with 
any other bit of IL before :-P

Let me think about it.  I am loathe to change range-ops itself, but we 
may be able to leverage the builtin-function approach to dealing with 
something non-standard. At least for the moment to keep you going.

For the builtins, we provide a range-ops handler, *after* we look at the 
operands from within a gimple-context where we can still see the types, 
and  choose an appropriate handler.  so I'm thinking we provide 2 handlers,

operator_widen_mult_signed
operator_widen_mult_unsigned

chosen based on whether to sign extned or zero extend op1. look at the 
type of operand one, and return the appropriate handler. Let me give you 
a skeleton.  I *think* this should do it.

you can provide 2 versions of  operator_widen_mult in range-ops (so you 
can still inherit from operator_mult).  The should be exported I think, 
and the appropriate one should be called I think...

Give it a try and see if it works :-P.





[-- Attachment #2: tamar.diff --]
[-- Type: text/x-patch, Size: 1586 bytes --]

diff --git a/gcc/gimple-range-op.cc b/gcc/gimple-range-op.cc
index d9dfdc56939..e4391f4a616 100644
--- a/gcc/gimple-range-op.cc
+++ b/gcc/gimple-range-op.cc
@@ -179,6 +179,8 @@ gimple_range_op_handler::gimple_range_op_handler (gimple *s)
   // statements.
   if (is_a <gcall *> (m_stmt))
     maybe_builtin_call ();
+  else
+    maybe_non_standard ();
 }
 
 // Calculate what we can determine of the range of this unary
@@ -764,6 +766,29 @@ public:
   }
 } op_cfn_parity;
 
+// Set up a gimple_range_op_handler for any nonstandard function which can be
+// supported via range-ops.
+
+void
+gimple_range_op_handler::maybe_non_standard ()
+{
+  if (gimple_code (m_stmt) == GIMPLE_ASSIGN)
+    switch (gimple_assign_rhs_code (m_stmt))
+      {
+	case WIDEN_MULT_EXPR:
+	  extern class range_operator &op_widen_mult_signed;
+	  extern class range_operator &op_widen_mult_unsigned;
+	  m_valid = true;
+	  m_op1 = gimple_assign_rhs1 (m_stmt);
+	  m_op2 = gimple_assign_rhs2 (m_stmt);
+	  if (TYPE_SIGN (TREE_TYPE (m_op1)) == SIGNED)
+	    m_int = &op_widen_mult_signed;
+	  else
+	    m_int = &op_widen_mult_unsigned;
+	default:
+	  break;
+      }
+}
 // Set up a gimple_range_op_handler for any built in function which can be
 // supported via range-ops.
 
diff --git a/gcc/gimple-range-op.h b/gcc/gimple-range-op.h
index 743b858126e..1bf63c5ce6f 100644
--- a/gcc/gimple-range-op.h
+++ b/gcc/gimple-range-op.h
@@ -41,6 +41,7 @@ public:
 		 relation_trio = TRIO_VARYING);
 private:
   void maybe_builtin_call ();
+  void maybe_non_standard ();
   gimple *m_stmt;
   tree m_op1, m_op2;
 };