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.to...@gmail.com> wrote:
> >> On Tue, Nov 25, 2014 at 7:01 AM, Richard Biener
> >> <richard.guent...@gmail.com> wrote:
> >>> On Tue, Nov 25, 2014 at 1:57 PM, H.J. Lu <hongjiu...@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?
Thanks.
H.J.
---
>From 7b274d517dcaae96f111652283d947c035ab7a22 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.to...@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;
+}
--
1.9.3
