Use DECL_ARG_TYPE for argument types in fnsplitting (PR tree-optimization/47148)

Hi!

The following testcase is miscompiled, because on x86_64 (and i386, but
given that it is a default behavior perhaps on other targets too)
the combiner expects arguments are properly zero/sign extended
by the caller from {,{,un}signed }{char,short} to int, but because of
fnsplitting the caller doesn't ensure it.  Normally GIMPLE_CALL arguments
use DECL_ARG_TYPE type, (for C c_type_promotes_to), but fnsplitting
was using just the normal type of the argument instead, which resulted
in a QImode pseudo being loaded with a value before the call, instead
of SImode.

Arguably the x86_64 backend is broken and should have the promotions
explicit, but as it affects other targets too, I think it is best
to also adjust fn splitting to do something similar to what the
FEs do.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
trunk?

2011-01-02  Jakub Jelinek  <jakub@redhat.com>

	PR tree-optimization/47148
	* ipa-split.c (split_function): Convert arguments to
	DECL_ARG_TYPE if possible.

	* gcc.c-torture/execute/pr47148.c: New test.


	Jakub

On Sun, Jan 2, 2011 at 6:18 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The following testcase is miscompiled, because on x86_64 (and i386, but
> given that it is a default behavior perhaps on other targets too)
> the combiner expects arguments are properly zero/sign extended
> by the caller from {,{,un}signed }{char,short} to int, but because of
> fnsplitting the caller doesn't ensure it.  Normally GIMPLE_CALL arguments
> use DECL_ARG_TYPE type, (for C c_type_promotes_to), but fnsplitting
> was using just the normal type of the argument instead, which resulted
> in a QImode pseudo being loaded with a value before the call, instead
> of SImode.
>
> Arguably the x86_64 backend is broken and should have the promotions
> explicit, but as it affects other targets too, I think it is best
> to also adjust fn splitting to do something similar to what the
> FEs do.  Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
> trunk?

Ick ...  I don't like

> +       arg = gimple_default_def (cfun, parm);
> +       if (!useless_type_conversion_p (DECL_ARG_TYPE (parm), TREE_TYPE (arg))
> +           && fold_convertible_p (DECL_ARG_TYPE (parm), arg))

this check.  Can we instead not do function splitting here?  And maybe
fix the x86 backend as well for a start?

If we want to do the promotion then we should use appropriate target
hooks to query whether we need to do one.

Richard.

> +         {
> +           conv_needed = true;
> +           arg = fold_convert (DECL_ARG_TYPE (parm), arg);
> +         }
> +       VEC_safe_push (tree, heap, args_to_pass, arg);
> +      }
>
>   /* See if the split function will return.  */
>   FOR_EACH_EDGE (e, ei, return_bb->preds)
> @@ -1056,6 +1068,14 @@ split_function (struct split_point *spli
>
>   /* Produce the call statement.  */
>   gsi = gsi_last_bb (call_bb);
> +  if (conv_needed)
> +    FOR_EACH_VEC_ELT (tree, args_to_pass, i, arg)
> +      if (!is_gimple_val (arg))
> +       {
> +         arg = force_gimple_operand_gsi (&gsi, arg, true, NULL_TREE,
> +                                         false, GSI_NEW_STMT);
> +         VEC_replace (tree, args_to_pass, i, arg);
> +       }
>   call = gimple_build_call_vec (node->decl, args_to_pass);
>   gimple_set_block (call, DECL_INITIAL (current_function_decl));
>
> --- gcc/testsuite/gcc.c-torture/execute/pr47148.c.jj    2011-01-02 12:24:41.000000000 +0100
> +++ gcc/testsuite/gcc.c-torture/execute/pr47148.c       2011-01-01 21:05:21.000000000 +0100
> @@ -0,0 +1,32 @@
> +/* PR tree-optimization/47148 */
> +
> +static inline unsigned
> +bar (unsigned x, unsigned y)
> +{
> +  if (y >= 32)
> +    return x;
> +  else
> +    return x >> y;
> +}
> +
> +static unsigned a = 1, b = 1;
> +
> +static inline void
> +foo (unsigned char x, unsigned y)
> +{
> +  if (!y)
> +    return;
> +  unsigned c = (0x7000U / (x - 2)) ^ a;
> +  unsigned d = bar (a, a);
> +  b &= ((a - d) && (a - 1)) + c;
> +}
> +
> +int
> +main (void)
> +{
> +  foo (1, 1);
> +  foo (-1, 1);
> +  if (b && ((unsigned char) -1) == 255)
> +    __builtin_abort ();
> +  return 0;
> +}
>
>        Jakub
>

On Sun, Jan 02, 2011 at 06:43:34PM +0100, Richard Guenther wrote:
> Ick ...  I don't like
> 
> > +       arg = gimple_default_def (cfun, parm);
> > +       if (!useless_type_conversion_p (DECL_ARG_TYPE (parm), TREE_TYPE (arg))
> > +           && fold_convertible_p (DECL_ARG_TYPE (parm), arg))
> 
> this check.  Can we instead not do function splitting here?  And maybe

You're worried just about the fold_convertible_p check, right?
If the conversion is useless, there is no point to add it.

It was added there just in case, I guess it could be an gcc_checking_assert
too, or such test could be moved to execute_split_function.  split_function
is already meant to be executed when we are committed to do the splitting.

> fix the x86 backend as well for a start?

x86_64?  Maybe.  I don't think we want to touch i?86 ABI (where the test
fails too), haven't tried other targets to see where else it fails as well.
I think DECL_ARG_TYPE isn't a target thing, it is something the FE decided
and the middle-end should honor it.  So it isn't something that should
be controlled by a target hook.

	Jakub

On Sun, Jan 2, 2011 at 8:07 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sun, Jan 02, 2011 at 06:43:34PM +0100, Richard Guenther wrote:
>> Ick ...  I don't like
>>
>> > +       arg = gimple_default_def (cfun, parm);
>> > +       if (!useless_type_conversion_p (DECL_ARG_TYPE (parm), TREE_TYPE (arg))
>> > +           && fold_convertible_p (DECL_ARG_TYPE (parm), arg))
>>
>> this check.  Can we instead not do function splitting here?  And maybe
>
> You're worried just about the fold_convertible_p check, right?
> If the conversion is useless, there is no point to add it.

I'm more worried about using a gimple IL type-consistency predicate
for a language ABI thing.  That said - the middle-end seems to be
totally unaware of DECL_ARG_TYPE, considering other transformations
that introduce new call statements (like autopar or omp).

It's a bad thing that we have an inconsistency between the caller and
callee side on the GIMPLE type side - of course nothing we can fix in
stage3.

I see that ipa-prop for example re-sets DECL_ARG_TYPE, something
we could also do for function splitting.  Ideally I'd like us to get rid of
all uses of DECL_ARG_TYPE from non-frontend parts (thus, make the
type of PARM_DECLs match what the frontend ABI specifies).

> It was added there just in case, I guess it could be an gcc_checking_assert
> too, or such test could be moved to execute_split_function.  split_function
> is already meant to be executed when we are committed to do the splitting.
>
>> fix the x86 backend as well for a start?
>
> x86_64?  Maybe.  I don't think we want to touch i?86 ABI (where the test
> fails too), haven't tried other targets to see where else it fails as well.
> I think DECL_ARG_TYPE isn't a target thing, it is something the FE decided
> and the middle-end should honor it.  So it isn't something that should
> be controlled by a target hook.

Right, but it's bad that throughout the tree transforms we hide that
information and instead operate on non-promoted types on the callee
side and on promoted types on the caller side.

As of your patch, I think it's reasonable at this stage - but please change
the predicate to

> +       arg = gimple_default_def (cfun, parm);
> +       if (TYPE_MAIN_VARIANT (DECL_ARG_TYPE (parm))
                != TYPE_MAIN_VARIANT (TREE_TYPE (arg)))

Thanks,
Richard.