This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On Fri, 20 Nov 2009, Martin Jambor wrote:

> 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 NOP_EXPR on the LHS is always invalid (but was that your question?).
Fold does not distinguish between SSA_NAMEs or bare decls and
happily folds V_C_E<signed int>(unsigned int) to a NOP_EXPR independent
of how the operand looks like (maybe that was it? ;)).

> > 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.

That sounds like overkill.  Why not avoid folding at all and not keep the
build1 (...) as it was if we process the LHS?

Richard.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]