| Message ID | 20141214140828.GA23100@gmail.com |
|---|---|
| State | New |
| Headers | show |
On December 14, 2014 3:08:28 PM CET, "H.J. Lu" <hjl.tools@gmail.com> wrote: >On Mon, Dec 08, 2014 at 11:16:58PM +0100, Uros Bizjak wrote: >> Hello! >> >> > On Tue, Nov 25, 2014 at 5:05 PM, H.J. Lu <hjl.tools@gmail.com> >wrote: >> >> On Tue, Nov 25, 2014 at 7:01 AM, Richard Biener >> >> <richard.guenther@gmail.com> wrote: >> >>> On Tue, Nov 25, 2014 at 1:57 PM, H.J. Lu <hongjiu.lu@intel.com> >wrote: >> >>>> Hi, >> >>>> >> >>>> The enclosed testcase fails on x86 when compiled with -Os since >we pass >> >>>> a byte parameter with a byte load in caller and read it as an >int in >> >>>> callee. The reason it only shows up with -Os is x86 backend >encodes >> >>>> a byte load with an int load if -O isn't used. When a byte load >is >> >>>> used, the upper 24 bits of the register have random value for >none >> >>>> WORD_REGISTER_OPERATIONS targets. >> >>>> >> >>>> It happens because setup_incoming_promotions in combine.c has >> >>>> >> >>>> /* The mode and signedness of the argument before any >promotions happen >> >>>> (equal to the mode of the pseudo holding it at that >stage). */ >> >>>> mode1 = TYPE_MODE (TREE_TYPE (arg)); >> >>>> uns1 = TYPE_UNSIGNED (TREE_TYPE (arg)); >> >>>> >> >>>> /* The mode and signedness of the argument after any >source language and >> >>>> TARGET_PROMOTE_PROTOTYPES-driven promotions. */ >> >>>> mode2 = TYPE_MODE (DECL_ARG_TYPE (arg)); >> >>>> uns3 = TYPE_UNSIGNED (DECL_ARG_TYPE (arg)); >> >>>> >> >>>> /* The mode and signedness of the argument as it is >actually passed, >> >>>> after any TARGET_PROMOTE_FUNCTION_ARGS-driven ABI >promotions. */ >> >>>> mode3 = promote_function_mode (DECL_ARG_TYPE (arg), mode2, >&uns3, >> >>>> TREE_TYPE (cfun->decl), 0); >> >>>> >> >>>> while they are actually passed in register by >assign_parm_setup_reg in >> >>>> function.c: >> >>>> >> >>>> /* Store the parm in a pseudoregister during the function, but >we may >> >>>> need to do it in a wider mode. Using 2 here makes the >result >> >>>> consistent with promote_decl_mode and thus >expand_expr_real_1. */ >> >>>> promoted_nominal_mode >> >>>> = promote_function_mode (data->nominal_type, >data->nominal_mode, &unsignedp, >> >>>> TREE_TYPE (current_function_decl), >2); >> >>>> >> >>>> where nominal_type and nominal_mode are set up with TREE_TYPE >(parm) >> >>>> and TYPE_MODE (nominal_type). TREE_TYPE here is >> >>> >> >>> I think the bug is here, not in combine.c. Can you try going >back in history >> >>> for both snippets and see if they matched at some point? >> >>> >> >> >> >> The bug was introduced by >> >> >> >> https://gcc.gnu.org/ml/gcc-cvs/2007-09/msg00613.html >> >> >> >> commit 5d93234932c3d8617ce92b77b7013ef6bede9508 >> >> Author: shinwell <shinwell@138bc75d-0d04-0410-961f-82ee72b054a4> >> >> Date: Thu Sep 20 11:01:18 2007 +0000 >> >> >> >> gcc/ >> >> * combine.c: Include cgraph.h. >> >> (setup_incoming_promotions): Rework to allow more aggressive >> >> elimination of sign extensions when all call sites of the >> >> current function are known to lie within the current unit. >> >> >> >> >> >> git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@128618 >> >> 138bc75d-0d04-0410-961f-82ee72b054a4 >> >> >> >> Before this commit, combine.c has >> >> >> >> enum machine_mode mode = TYPE_MODE (TREE_TYPE (arg)); >> >> int uns = TYPE_UNSIGNED (TREE_TYPE (arg)); >> >> >> >> mode = promote_mode (TREE_TYPE (arg), mode, &uns, 1); >> >> if (mode == GET_MODE (reg) && mode != DECL_MODE (arg)) >> >> { >> >> rtx x; >> >> x = gen_rtx_CLOBBER (DECL_MODE (arg), const0_rtx); >> >> x = gen_rtx_fmt_e ((uns ? ZERO_EXTEND : >SIGN_EXTEND), mode, x); >> >> record_value_for_reg (reg, first, x); >> >> } >> >> >> >> It matches function.c: >> >> >> >> /* This is not really promoting for a call. However we need to >be >> >> consistent with assign_parm_find_data_types and >expand_expr_real_1. */ >> >> promoted_nominal_mode >> >> = promote_mode (data->nominal_type, data->nominal_mode, >&unsignedp, 1); >> >> >> >> r128618 changed >> >> >> >> mode = promote_mode (TREE_TYPE (arg), mode, &uns, 1); >> >> >> >> to >> >> >> >> mode3 = promote_mode (DECL_ARG_TYPE (arg), mode2, &uns3, 1); >> >> >> >> It breaks none WORD_REGISTER_OPERATIONS targets. >> > >> > Hmm, I think that DECL_ARG_TYPE makes a difference only >> > for non-WORD_REGISTER_OPERATIONS targets. >> > >> > But yeah, isolated the above change looks wrong. >> > >> > Your patch is ok for trunk if nobody objects within 24h and for >branches >> > after a week. >> > >> > Thanks, >> > Richard. >> >> This patch caused PR64213. >> > >Here is the updated patch. The difference is > > mode3 = promote_function_mode (TREE_TYPE (arg), mode1, &uns3, > TREE_TYPE (cfun->decl), 0); > >vs > > mode3 = promote_function_mode (TREE_TYPE (arg), mode1, &uns1, > TREE_TYPE (cfun->decl), 0); > >I made a mistake in my previous patch where I shouldn't have changed >&uns3 to &uns1. We do want to update mode3 and uns3, not mode3 and >uns1. It generates the same code on PR64213 testcase with a cross >alpha-linux GCC. > >Uros, can you test it on Linux/alpha? OK for master, 4.9 and 4.8 >branches if it works on Linux/alpha? OK for trunk and 4.9, please wait until after 4.8.4 is released for the 4.8 branch. Thanks, Richard. >Thanks. > > >H.J. >--- >From 7b274d517dcaae96f111652283d947c035ab7a22 Mon Sep 17 00:00:00 2001 >From: "H.J. Lu" <hjl.tools@gmail.com> >Date: Sun, 14 Dec 2014 05:37:41 -0800 >Subject: [PATCH] Pass unpromoted argument to promote_function_mode > >This patch updates setup_incoming_promotions in combine.c to match what >is actually passed in assign_parm_setup_reg in function.c. > >gcc/ > > PR rtl-optimization/64037 > * combine.c (setup_incoming_promotions): Pass the argument > before any promotions happen to promote_function_mode. > >gcc/testsuite/ > > PR rtl-optimization/64037 > * g++.dg/pr64037.C: New test. >--- >diff --git a/gcc/combine.c b/gcc/combine.c >index c95b493..ee7b3f9 100644 >--- a/gcc/combine.c >+++ b/gcc/combine.c >@@ -1579,8 +1579,8 @@ setup_incoming_promotions (rtx_insn *first) > uns3 = TYPE_UNSIGNED (DECL_ARG_TYPE (arg)); > > /* The mode and signedness of the argument as it is actually passed, >- after any TARGET_PROMOTE_FUNCTION_ARGS-driven ABI promotions. > */ >- mode3 = promote_function_mode (DECL_ARG_TYPE (arg), mode2, >&uns3, >+ see assign_parm_setup_reg in function.c. */ >+ mode3 = promote_function_mode (TREE_TYPE (arg), mode1, &uns3, > TREE_TYPE (cfun->decl), 0); > > /* The mode of the register in which the argument is being passed. */ >diff --git a/gcc/testsuite/g++.dg/pr64037.C >b/gcc/testsuite/g++.dg/pr64037.C >new file mode 100644 >index 0000000..e5cd0e2 >--- /dev/null >+++ b/gcc/testsuite/g++.dg/pr64037.C >@@ -0,0 +1,27 @@ >+// { dg-do run { target i?86-*-* x86_64-*-* } } >+// { dg-options "-std=c++11 -Os" } >+ >+enum class X : unsigned char { >+ V = 2, >+}; >+ >+static void >+__attribute__((noinline,noclone)) >+foo(unsigned &out, unsigned a, X b) >+{ >+ out = static_cast<unsigned>(b); >+} >+ >+int main() >+{ >+ unsigned deadbeef = 0xDEADBEEF; >+ asm volatile ("" : "+d" (deadbeef), "+c" (deadbeef)); >+ >+ unsigned out; >+ foo(out, 2, X::V); >+ >+ if (out != 2) >+ __builtin_abort (); >+ >+ return 0; >+}
On Sun, Dec 14, 2014 at 3:08 PM, H.J. Lu <hjl.tools@gmail.com> wrote: >> >>>> The enclosed testcase fails on x86 when compiled with -Os since we pass >> >>>> a byte parameter with a byte load in caller and read it as an int in >> >>>> callee. The reason it only shows up with -Os is x86 backend encodes >> >>>> a byte load with an int load if -O isn't used. When a byte load is >> >>>> used, the upper 24 bits of the register have random value for none >> >>>> WORD_REGISTER_OPERATIONS targets. >> >>>> >> >>>> It happens because setup_incoming_promotions in combine.c has >> >>>> >> >>>> /* The mode and signedness of the argument before any promotions happen >> >>>> (equal to the mode of the pseudo holding it at that stage). */ >> >>>> mode1 = TYPE_MODE (TREE_TYPE (arg)); >> >>>> uns1 = TYPE_UNSIGNED (TREE_TYPE (arg)); >> >>>> >> >>>> /* The mode and signedness of the argument after any source language and >> >>>> TARGET_PROMOTE_PROTOTYPES-driven promotions. */ >> >>>> mode2 = TYPE_MODE (DECL_ARG_TYPE (arg)); >> >>>> uns3 = TYPE_UNSIGNED (DECL_ARG_TYPE (arg)); >> >>>> >> >>>> /* The mode and signedness of the argument as it is actually passed, >> >>>> after any TARGET_PROMOTE_FUNCTION_ARGS-driven ABI promotions. */ >> >>>> mode3 = promote_function_mode (DECL_ARG_TYPE (arg), mode2, &uns3, >> >>>> TREE_TYPE (cfun->decl), 0); >> >>>> >> >>>> while they are actually passed in register by assign_parm_setup_reg in >> >>>> function.c: >> >>>> >> >>>> /* Store the parm in a pseudoregister during the function, but we may >> >>>> need to do it in a wider mode. Using 2 here makes the result >> >>>> consistent with promote_decl_mode and thus expand_expr_real_1. */ >> >>>> promoted_nominal_mode >> >>>> = promote_function_mode (data->nominal_type, data->nominal_mode, &unsignedp, >> >>>> TREE_TYPE (current_function_decl), 2); >> >>>> >> >>>> where nominal_type and nominal_mode are set up with TREE_TYPE (parm) >> >>>> and TYPE_MODE (nominal_type). TREE_TYPE here is >> >>> >> >>> I think the bug is here, not in combine.c. Can you try going back in history >> >>> for both snippets and see if they matched at some point? >> >>> >> >> >> >> The bug was introduced by >> >> >> >> https://gcc.gnu.org/ml/gcc-cvs/2007-09/msg00613.html >> >> >> >> commit 5d93234932c3d8617ce92b77b7013ef6bede9508 >> >> Author: shinwell <shinwell@138bc75d-0d04-0410-961f-82ee72b054a4> >> >> Date: Thu Sep 20 11:01:18 2007 +0000 >> >> >> >> gcc/ >> >> * combine.c: Include cgraph.h. >> >> (setup_incoming_promotions): Rework to allow more aggressive >> >> elimination of sign extensions when all call sites of the >> >> current function are known to lie within the current unit. >> >> >> >> >> >> git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@128618 >> >> 138bc75d-0d04-0410-961f-82ee72b054a4 >> >> >> >> Before this commit, combine.c has >> >> >> >> enum machine_mode mode = TYPE_MODE (TREE_TYPE (arg)); >> >> int uns = TYPE_UNSIGNED (TREE_TYPE (arg)); >> >> >> >> mode = promote_mode (TREE_TYPE (arg), mode, &uns, 1); >> >> if (mode == GET_MODE (reg) && mode != DECL_MODE (arg)) >> >> { >> >> rtx x; >> >> x = gen_rtx_CLOBBER (DECL_MODE (arg), const0_rtx); >> >> x = gen_rtx_fmt_e ((uns ? ZERO_EXTEND : SIGN_EXTEND), mode, x); >> >> record_value_for_reg (reg, first, x); >> >> } >> >> >> >> It matches function.c: >> >> >> >> /* This is not really promoting for a call. However we need to be >> >> consistent with assign_parm_find_data_types and expand_expr_real_1. */ >> >> promoted_nominal_mode >> >> = promote_mode (data->nominal_type, data->nominal_mode, &unsignedp, 1); >> >> >> >> r128618 changed >> >> >> >> mode = promote_mode (TREE_TYPE (arg), mode, &uns, 1); >> >> >> >> to >> >> >> >> mode3 = promote_mode (DECL_ARG_TYPE (arg), mode2, &uns3, 1); >> >> >> >> It breaks none WORD_REGISTER_OPERATIONS targets. >> > >> > Hmm, I think that DECL_ARG_TYPE makes a difference only >> > for non-WORD_REGISTER_OPERATIONS targets. >> > >> > But yeah, isolated the above change looks wrong. >> > >> > Your patch is ok for trunk if nobody objects within 24h and for branches >> > after a week. >> > >> > Thanks, >> > Richard. >> >> This patch caused PR64213. >> > > Here is the updated patch. The difference is > > mode3 = promote_function_mode (TREE_TYPE (arg), mode1, &uns3, > TREE_TYPE (cfun->decl), 0); > > vs > > mode3 = promote_function_mode (TREE_TYPE (arg), mode1, &uns1, > TREE_TYPE (cfun->decl), 0); > > I made a mistake in my previous patch where I shouldn't have changed > &uns3 to &uns1. We do want to update mode3 and uns3, not mode3 and > uns1. It generates the same code on PR64213 testcase with a cross > alpha-linux GCC. > > Uros, can you test it on Linux/alpha? OK for master, 4.9 and 4.8 > branches if it works on Linux/alpha? Yes, this patch works OK [1] on linux/alpha mainline. [1] https://gcc.gnu.org/ml/gcc-testresults/2014-12/msg01867.html Uros.
On Mon, Dec 15, 2014 at 9:29 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >>> >>>> The enclosed testcase fails on x86 when compiled with -Os since we pass >>> >>>> a byte parameter with a byte load in caller and read it as an int in >>> >>>> callee. The reason it only shows up with -Os is x86 backend encodes >>> >>>> a byte load with an int load if -O isn't used. When a byte load is >>> >>>> used, the upper 24 bits of the register have random value for none >>> >>>> WORD_REGISTER_OPERATIONS targets. >>> >>>> >>> >>>> It happens because setup_incoming_promotions in combine.c has >>> >>>> >>> >>>> /* The mode and signedness of the argument before any promotions happen >>> >>>> (equal to the mode of the pseudo holding it at that stage). */ >>> >>>> mode1 = TYPE_MODE (TREE_TYPE (arg)); >>> >>>> uns1 = TYPE_UNSIGNED (TREE_TYPE (arg)); >>> >>>> >>> >>>> /* The mode and signedness of the argument after any source language and >>> >>>> TARGET_PROMOTE_PROTOTYPES-driven promotions. */ >>> >>>> mode2 = TYPE_MODE (DECL_ARG_TYPE (arg)); >>> >>>> uns3 = TYPE_UNSIGNED (DECL_ARG_TYPE (arg)); >>> >>>> >>> >>>> /* The mode and signedness of the argument as it is actually passed, >>> >>>> after any TARGET_PROMOTE_FUNCTION_ARGS-driven ABI promotions. */ >>> >>>> mode3 = promote_function_mode (DECL_ARG_TYPE (arg), mode2, &uns3, >>> >>>> TREE_TYPE (cfun->decl), 0); >>> >>>> >>> >>>> while they are actually passed in register by assign_parm_setup_reg in >>> >>>> function.c: >>> >>>> >>> >>>> /* Store the parm in a pseudoregister during the function, but we may >>> >>>> need to do it in a wider mode. Using 2 here makes the result >>> >>>> consistent with promote_decl_mode and thus expand_expr_real_1. */ >>> >>>> promoted_nominal_mode >>> >>>> = promote_function_mode (data->nominal_type, data->nominal_mode, &unsignedp, >>> >>>> TREE_TYPE (current_function_decl), 2); >>> >>>> >>> >>>> where nominal_type and nominal_mode are set up with TREE_TYPE (parm) >>> >>>> and TYPE_MODE (nominal_type). TREE_TYPE here is >>> >>> >>> >>> I think the bug is here, not in combine.c. Can you try going back in history >>> >>> for both snippets and see if they matched at some point? >>> >>> >>> >> >>> >> The bug was introduced by >>> >> >>> >> https://gcc.gnu.org/ml/gcc-cvs/2007-09/msg00613.html >>> >> >>> >> commit 5d93234932c3d8617ce92b77b7013ef6bede9508 >>> >> Author: shinwell <shinwell@138bc75d-0d04-0410-961f-82ee72b054a4> >>> >> Date: Thu Sep 20 11:01:18 2007 +0000 >>> >> >>> >> gcc/ >>> >> * combine.c: Include cgraph.h. >>> >> (setup_incoming_promotions): Rework to allow more aggressive >>> >> elimination of sign extensions when all call sites of the >>> >> current function are known to lie within the current unit. >>> >> >>> >> >>> >> git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@128618 >>> >> 138bc75d-0d04-0410-961f-82ee72b054a4 >>> >> >>> >> Before this commit, combine.c has >>> >> >>> >> enum machine_mode mode = TYPE_MODE (TREE_TYPE (arg)); >>> >> int uns = TYPE_UNSIGNED (TREE_TYPE (arg)); >>> >> >>> >> mode = promote_mode (TREE_TYPE (arg), mode, &uns, 1); >>> >> if (mode == GET_MODE (reg) && mode != DECL_MODE (arg)) >>> >> { >>> >> rtx x; >>> >> x = gen_rtx_CLOBBER (DECL_MODE (arg), const0_rtx); >>> >> x = gen_rtx_fmt_e ((uns ? ZERO_EXTEND : SIGN_EXTEND), mode, x); >>> >> record_value_for_reg (reg, first, x); >>> >> } >>> >> >>> >> It matches function.c: >>> >> >>> >> /* This is not really promoting for a call. However we need to be >>> >> consistent with assign_parm_find_data_types and expand_expr_real_1. */ >>> >> promoted_nominal_mode >>> >> = promote_mode (data->nominal_type, data->nominal_mode, &unsignedp, 1); >>> >> >>> >> r128618 changed >>> >> >>> >> mode = promote_mode (TREE_TYPE (arg), mode, &uns, 1); >>> >> >>> >> to >>> >> >>> >> mode3 = promote_mode (DECL_ARG_TYPE (arg), mode2, &uns3, 1); >>> >> >>> >> It breaks none WORD_REGISTER_OPERATIONS targets. >>> > >>> > Hmm, I think that DECL_ARG_TYPE makes a difference only >>> > for non-WORD_REGISTER_OPERATIONS targets. >>> > >>> > But yeah, isolated the above change looks wrong. >>> > >>> > Your patch is ok for trunk if nobody objects within 24h and for branches >>> > after a week. >>> > >>> > Thanks, >>> > Richard. >>> >>> This patch caused PR64213. >>> >> >> Here is the updated patch. The difference is >> >> mode3 = promote_function_mode (TREE_TYPE (arg), mode1, &uns3, >> TREE_TYPE (cfun->decl), 0); >> >> vs >> >> mode3 = promote_function_mode (TREE_TYPE (arg), mode1, &uns1, >> TREE_TYPE (cfun->decl), 0); >> >> I made a mistake in my previous patch where I shouldn't have changed >> &uns3 to &uns1. We do want to update mode3 and uns3, not mode3 and >> uns1. It generates the same code on PR64213 testcase with a cross >> alpha-linux GCC. >> >> Uros, can you test it on Linux/alpha? OK for master, 4.9 and 4.8 >> branches if it works on Linux/alpha? > > Yes, this patch works OK [1] on linux/alpha mainline. ... and 4.9 branch [2]. > [1] https://gcc.gnu.org/ml/gcc-testresults/2014-12/msg01867.html [2] https://gcc.gnu.org/ml/gcc-testresults/2014-12/msg01919.html Uros.
diff --git a/gcc/combine.c b/gcc/combine.c index c95b493..ee7b3f9 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -1579,8 +1579,8 @@ setup_incoming_promotions (rtx_insn *first) uns3 = TYPE_UNSIGNED (DECL_ARG_TYPE (arg)); /* The mode and signedness of the argument as it is actually passed, - after any TARGET_PROMOTE_FUNCTION_ARGS-driven ABI promotions. */ - mode3 = promote_function_mode (DECL_ARG_TYPE (arg), mode2, &uns3, + see assign_parm_setup_reg in function.c. */ + mode3 = promote_function_mode (TREE_TYPE (arg), mode1, &uns3, TREE_TYPE (cfun->decl), 0); /* The mode of the register in which the argument is being passed. */ diff --git a/gcc/testsuite/g++.dg/pr64037.C b/gcc/testsuite/g++.dg/pr64037.C new file mode 100644 index 0000000..e5cd0e2 --- /dev/null +++ b/gcc/testsuite/g++.dg/pr64037.C @@ -0,0 +1,27 @@ +// { dg-do run { target i?86-*-* x86_64-*-* } } +// { dg-options "-std=c++11 -Os" } + +enum class X : unsigned char { + V = 2, +}; + +static void +__attribute__((noinline,noclone)) +foo(unsigned &out, unsigned a, X b) +{ + out = static_cast<unsigned>(b); +} + +int main() +{ + unsigned deadbeef = 0xDEADBEEF; + asm volatile ("" : "+d" (deadbeef), "+c" (deadbeef)); + + unsigned out; + foo(out, 2, X::V); + + if (out != 2) + __builtin_abort (); + + return 0; +}