[PATCH][2/2][RFC] Fix PR49806, promote/demote binary operations in VRP

[Richard Guenther]

On Tue, 2 Aug 2011, Ira Rosen wrote:

> 
> 
> Richard Guenther <rguenther@suse.de> wrote on 02/08/2011 01:33:49 PM:
> >
> > On Tue, 2 Aug 2011, Ira Rosen wrote:
> >
> > >
> > > > +   /* Now we have matched the statement pattern
> > > > +
> > > > +        rhs1 = (T1)x;
> > > > +        rhs2 = (T1)y;
> > > > +        op_result = rhs1 OP rhs2;
> > > > +        lhs = (T2)op_result;
> > >
> > > Just a note that the patch I proposed for the vectorizer (
> > > http://gcc.gnu.org/ml/gcc-patches/2011-07/msg01472.html) also handles
> > > constants, multiple statements (i.e., op_result doesn't have to be
> promoted
> > > itself, but the sequence needs to end up with a promotion), and also it
> may
> > > use an intermediate type for OP. The tests in my patch don't match the
> > > pattern this patch detects.
> >
> > Ok, I only looked at the description of your patch, not the patch itself.
> >
> > The patch already handles constant 2nd operands.
> >
> > It shouldn't be difficult to handle multiple statements here, either by
> > instead of the above match only
> >
> >         op_result = rhs1 OP rhs2;
> >         lhs = (T2)op_result;
> >
> > and thus allow iteration on the promoted/demoted operation operands
> > or by collecting all defs first.
> >
> > Do you handle arbitrary def trees or only a linear chain as suggested
> > by
> >
> > +     S2  x_T = (TYPE) x_t;
> > +     S3  res0_T = op (x_T, C0);
> > +     S4  res1_T = op (res0_T, C1);
> > +     S5  ... = () res1_T;  - type demotion
> >
> > ?  Thus, do you handle res1_T = op (res0_T, res2_T) with a possibly
> > different TYPE in its def?
> 
> Only linear chains. But it doesn't seem too complicated to only check if
> res2_T is a result of a type promotion.

Thinking about it it probably makes sense to keep a variant of this
in the vectorizer - after all it has quite specific requirements on
operand sizes while VRP would probably demote as far as possible
(maybe taking PROMOTE_MODE into account).

A quick look at your patch reveals

+  if (gimple_assign_rhs_code (use_stmt) == CONVERT_EXPR)

CONVERT_EXPR_CODE_P (gimple_assign_rhs_code (use_stmt))

+          tmp = create_tmp_var (use_type, NULL);

create_tmp_reg

+  if (!types_compatible_p (TREE_TYPE (oprnd0), type)
+      || !types_compatible_p (TREE_TYPE (oprnd1), type)
+      || (TREE_CODE (oprnd0) != INTEGER_CST
+          && TREE_CODE (oprnd1) != INTEGER_CST))

it's always the second operand that is constant, you can simplify
the code to not handle CST op SSA.

+  code = gimple_assign_rhs_code (stmt);
+  if (code != LSHIFT_EXPR && code != RSHIFT_EXPR
+      && code != BIT_IOR_EXPR && code != BIT_XOR_EXPR && code != 
BIT_AND_EXPR)
+    return false;
+
+  oprnd0 = gimple_assign_rhs1 (stmt);
+  oprnd1 = gimple_assign_rhs2 (stmt);
+  type = gimple_expr_type (stmt);
+  if (!types_compatible_p (TREE_TYPE (oprnd0), type)
+      || !types_compatible_p (TREE_TYPE (oprnd1), type)

for shifts the type compatibility check of oprnd1 isn't guaranteed
(but do we care?  we only will handle constant shift amounts), for
the other operands of the codes you handle they always return true.

So I'd simplify the check to

  if (TREE_CODE (oprnd0) != SSA_NAME
      || TREE_CODE (oprnd1) != INTEGER_CST)
    return false;

Otherwise the patch looks sensible.

Richard.