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

login
register
mail settings
Submitter Jakub Jelinek
Date Jan. 2, 2011, 5:18 p.m.
Message ID <20110102171825.GZ16156@tyan-ft48-01.lab.bos.redhat.com>
Download mbox | patch
Permalink /patch/77195/
State New
Headers show

Comments

Jakub Jelinek - Jan. 2, 2011, 5:18 p.m.
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
Richard Guenther - Jan. 2, 2011, 5:43 p.m.
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
>
Jakub Jelinek - Jan. 2, 2011, 7:07 p.m.
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
Richard Guenther - Jan. 3, 2011, 5:40 p.m.
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.

Patch

--- gcc/ipa-split.c.jj	2010-11-15 09:28:21.000000000 +0100
+++ gcc/ipa-split.c	2011-01-02 12:13:43.000000000 +0100
@@ -943,6 +943,9 @@  split_function (struct split_point *spli
   tree retval = NULL, real_retval = NULL;
   bool split_part_return_p = false;
   gimple last_stmt = NULL;
+  bool conv_needed = false;
+  unsigned int i;
+  tree arg;
 
   if (dump_file)
     {
@@ -959,7 +962,16 @@  split_function (struct split_point *spli
 			  SSA_NAME_VERSION (gimple_default_def (cfun, parm))))
       bitmap_set_bit (args_to_skip, num);
     else
-      VEC_safe_push (tree, heap, args_to_pass, gimple_default_def (cfun, parm));
+      {
+	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))
+	  {
+	    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;
+}