This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: PATCH: PR rtl-optimization/64037: Miscompilation with -Os and enum class : char parameter
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: "H.J. Lu" <hjl dot tools at gmail dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Eric Botcazou <ebotcazou at adacore dot com>
- Date: Tue, 25 Nov 2014 16:04:28 +0100
- Subject: Re: PATCH: PR rtl-optimization/64037: Miscompilation with -Os and enum class : char parameter
- Authentication-results: sourceware.org; auth=none
- References: <20141125125711 dot GA9855 at intel dot com> <CAFiYyc1VzJENyXPGC=n4vEqXvAcLv6L7JLHiJcfi2MQmUY6UqQ at mail dot gmail dot com>
On Tue, Nov 25, 2014 at 4:01 PM, 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?
Oh, and note that I think DECL_ARG_TYPE is sth dangerous - it's meant
to be a source language "ABI" kind-of-thing. Or rather an optimization
hit. For example in C when integral promotions happen to call arguments
this can be used to optimize sign-/zero-extensions in the callee. Unless
something else overrides this (like the target which specifies the real ABI).
IIRC.
Richard.
> Thanks,
> Richard.
>
>> (gdb) call debug_tree (type)
>> <enumeral_type 0x7ffff19f85e8 X
>> type <integer_type 0x7ffff18a93f0 unsigned char public unsigned
>> string-flag QI
>> size <integer_cst 0x7ffff18a5fa8 constant 8>
>> unit size <integer_cst 0x7ffff18a5fc0 constant 1>
>> align 8 symtab 0 alias set -1 canonical type 0x7ffff18a93f0
>> precision 8 min <integer_cst 0x7ffff18a5fd8 0> max <integer_cst
>> 0x7ffff18a5f78 255>>
>> static unsigned type_5 QI size <integer_cst 0x7ffff18a5fa8 8> unit
>> size <integer_cst 0x7ffff18a5fc0 1>
>> align 8 symtab 0 alias set -1 canonical type 0x7ffff19f85e8
>> precision 8 min <integer_cst 0x7ffff18a5fd8 0> max <integer_cst
>> 0x7ffff18a5f78 255>
>> values <tree_list 0x7ffff19fb028
>> purpose <identifier_node 0x7ffff19f6738 V
>> bindings <(nil)>
>> local bindings <(nil)>>
>> value <const_decl 0x7ffff18c21c0 V type <enumeral_type
>> 0x7ffff19f85e8 X>
>> readonly constant used VOID file pr64037.ii line 2 col 3
>> align 1 context <enumeral_type 0x7ffff19f85e8 X> initial
>> <integer_cst 0x7ffff19d8d08 2>>> context <translation_unit_decl
>> 0x7ffff7ff91e0 D.1>
>> chain <type_decl 0x7ffff19f5c78 X>>
>> (gdb)
>>
>> and DECL_ARG_TYPE is
>>
>> (gdb) call debug_tree (type)
>> <integer_type 0x7ffff18a9690 int public SI
>> size <integer_cst 0x7ffff18a5e70 type <integer_type 0x7ffff18a9150
>> bitsizetype> constant 32>
>> unit size <integer_cst 0x7ffff18a5e88 type <integer_type
>> 0x7ffff18a90a8 sizetype> constant 4>
>> align 32 symtab 0 alias set 1 canonical type 0x7ffff18a9690
>> precision 32 min <integer_cst 0x7ffff18c60c0 -2147483648> max
>> <integer_cst 0x7ffff18c60d8 2147483647>
>> pointer_to_this <pointer_type 0x7ffff18cb930>>
>> (gdb)
>>
>> This mismatch makes combine thinks a byte parameter is passed as int
>> in register and turns
>>
>> (insn 9 6 10 2 (set (reg:SI 92 [ b ])
>> (zero_extend:SI (subreg:QI (reg:SI 91 [ b ]) 0))) pr64037.ii:9
>> 138 {*zero_extendqisi2}
>> (expr_list:REG_DEAD (reg:SI 91 [ b ])
>> (nil)))
>> (insn 10 9 0 2 (set (mem:SI (reg/v/f:SI 88 [ out ]) [1 *out_4(D)+0 S4
>> A32])
>> (reg:SI 92 [ b ])) pr64037.ii:9 90 {*movsi_internal}
>> (expr_list:REG_DEAD (reg:SI 92 [ b ])
>> (expr_list:REG_DEAD (reg/v/f:SI 88 [ out ])
>> (nil))))
>>
>> into
>>
>> Trying 9 -> 10:
>> Successfully matched this instruction:
>> (set (mem:SI (reg/v/f:SI 88 [ out ]) [1 *out_4(D)+0 S4 A32])
>> (reg:SI 91 [ b ]))
>> allowing combination of insns 9 and 10
>> original costs 6 + 4 = 10
>> replacement cost 4
>> deferring deletion of insn with uid = 9.
>> modifying insn i3 10: [r88:SI]=r91:SI
>> REG_DEAD r91:SI
>> REG_DEAD r88:SI
>>
>>
>> This patch makes setup_incoming_promotions to match assign_parm_setup_reg.
>> Tested on Linux/x86-64 without regressions. OK for trunk and backport?
>>
>> Thanks.
>>
>>
>> H.J.
>> ----
>> diff --git a/gcc/combine.c b/gcc/combine.c
>> index 1808f97..a0449a2 100644
>> --- a/gcc/combine.c
>> +++ b/gcc/combine.c
>> @@ -1561,8 +1561,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, &uns1,
>> 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;
>> +}