Message ID | 20110102171825.GZ16156@tyan-ft48-01.lab.bos.redhat.com |
---|---|
State | New |
Headers | show |
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.
--- 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; +}