Re: [PATCH, PR 42025] Really ensuring type compatibilty in IL generated by IPA-SRA

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Fri, Nov 20, 2009 at 12:55:39PM +0100, Richard Guenther wrote:
> On Thu, 19 Nov 2009, Martin Jambor wrote:
> 
> > -/* Callback for scan_function.  If the expression *EXPR should be replaced by a
> > -   reduction of a parameter, do so.  DATA is a pointer to a vector of
> > -   adjustments.  */
> > +/* Callback for scan_function and helper to sra_ipa_modify_assign.  If the
> > +   expression *EXPR should be replaced by a reduction of a parameter, do so.
> > +   DATA is a pointer to a vector of adjustments.  DONT_CONVERT specifies
> > +   whether the function should care about type incompatibility the current and
> > +   new expressions.  If it is true, the function will leave incompatibility
> > +   issues to the caller.
> > +
> > +   When called directly by scan_function, DONT_CONVERT is true when the EXPR is
> > +   a write (LHS) expression.  */
> >  
> >  static bool
> >  sra_ipa_modify_expr (tree *expr, gimple_stmt_iterator *gsi ATTRIBUTE_UNUSED,
> > -		     bool write ATTRIBUTE_UNUSED, void *data)
> > +		     bool dont_convert, void *data)
> >  {
> >    ipa_parm_adjustment_vec adjustments;
> >    int i, len;
> > @@ -3543,10 +3575,10 @@ sra_ipa_modify_expr (tree *expr, gimple_
> >    if (TREE_CODE (*expr) == BIT_FIELD_REF
> >        || TREE_CODE (*expr) == IMAGPART_EXPR
> >        || TREE_CODE (*expr) == REALPART_EXPR)
> > -    expr = &TREE_OPERAND (*expr, 0);
> > -  while (TREE_CODE (*expr) == NOP_EXPR
> > -	 || TREE_CODE (*expr) == VIEW_CONVERT_EXPR)
> > -    expr = &TREE_OPERAND (*expr, 0);
> > +    {
> > +      expr = &TREE_OPERAND (*expr, 0);
> > +      dont_convert = false;
> > +    }
> >  
> >    base = get_ref_base_and_extent (*expr, &offset, &size, &max_size);
> >    if (!base || size == -1 || max_size == -1)
> > @@ -3594,13 +3626,15 @@ sra_ipa_modify_expr (tree *expr, gimple_
> >        fprintf (dump_file, "\n");
> >      }
> >  
> > -  if (!useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
> > +  if (!dont_convert &&
> > +      !useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
> >      {
> > -      tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*expr), src);
> > +      tree vce = fold_build1_loc (EXPR_LOCATION (*expr), VIEW_CONVERT_EXPR,
> > +				  TREE_TYPE (*expr), src);
> 
> This doesn't look correct.  Above you may end up setting dont_convert
> for the LHS where you may not fold the VIEW_CONVERT_EXPR you generate
> (because it might end up being a NOP_EXPR which is invalid on the LHS).

Thanks for the comment, this might indeed probably be a problem.

Is it always invalid on the LHS or only when the argument is not a
gimple register (for example when it has DECL_GIMPLE_REG_P == 0 which
complex replacements would currently have)?  I thought this was only
problem if the conversion argument was an SSA_NAME.

> A union with a complex signed integer entry and a struct with
> two unsigned ints and a REALPART_EXPR on it should trigger that
> (well, if carefully crafted).

The problem may not occur with a union of a complex and a structure
because the comparison function compare_access_positions we use for
sorting always prefers gimple register types over aggregates, so the
selected type of the replacements will always be gimple register type
(if any is involved and complex numbers are gimple register types).
Nevertheless, the conversion could still happen if we had a union of
two incompatible gimple registers of the same size and at least one of
them was a complex number.

So I was thinking, if conversions of non-gimple registers were ok on
the left hand side, we could change compare_access_positions to prefer
complex types over the other gimple types.  That way, the
replacement would always have DECL_GIMPLE_REG_P equal to zero.

But I guess it all depends on the first question.  If conversions on
the LHS are a problem even for non-gimple registers, I will probably
disable IPA-SRA for parameters which have an access group with a
partial lhs access and multiple different types.

> 
> Remove the redundant set of any to true below.

I must have sent an old version of the patch.  I's already removed.

Thanks,

Martin