Re: [PATCH] tree-optimization/114074 - CHREC multiplication and undefined overflow

[Jakub Jelinek <jakub@redhat.com>]

On Mon, Feb 26, 2024 at 03:29:30PM +0100, Richard Biener wrote:
> On Mon, 26 Feb 2024, Jakub Jelinek wrote:
> 
> > On Mon, Feb 26, 2024 at 03:15:02PM +0100, Richard Biener wrote:
> > > When folding a multiply CHRECs are handled like {a, +, b} * c
> > > is {a*c, +, b*c} but that isn't generally correct when overflow
> > > invokes undefined behavior.  The following uses unsigned arithmetic
> > > unless either a is zero or a and b have the same sign.
> > > 
> > > I've used simple early outs for INTEGER_CSTs and otherwise use
> > > a range-query since we lack a tree_expr_nonpositive_p.
> > 
> > What about testing
> >     (get_range_pos_neg (CHREC_LEFT (op0))
> >      | get_range_pos_neg (CHREC_RIGHT (op0))) != 3
> > ?
> 
> Ah, didn't know about that.  It seems to treat zero as "always
> positive", so for 0 and -1 I'd get 3.  OK as I check for
> zero CHREC_LEFT separately.

The function has been used where one cares about the possible values
of the sign bit, so 0 in that case is 0 sign bit.
If you want to differentiate between negative, 0 and positive and allow
non-positive with non-positive or non-negative with non-negative together,
then it isn't the right function to call (unless we add tracking if the
value can be zero, return bitmask with 3 bits rather than 2 and adjust
all callers).

> I'll note that get_range_pos_neg only asks global range query
> and for SSA names (but not sure if range_of_expr handles aribitrary
> GENERIC as SCEV tends to have here ...).

I think range_of_expr should handle usual GENERIC trees and punt on stuff it
doesn't handle.  And your patch was using ranger from current pass while
get_range_pos_neg uses always the global ranges (it is usually used during
expansion where the pass doesn't have its ranger instance).
Though, if you don't ask for range on a particular statement, it doesn't
really matter and is just a global range query.

> Will update the patch, I think any improvement should be done
> to get_range_pos_neg (it's a bit odd in behavior for unsigned
> but I have only signed things incoming).

For unsigned if it always returned 1, it would be quite useless, there would
be no point for the caller to call it in that case.

	Jakub