PATCH: PR rtl-optimization/64037: Miscompilation with -Os and enum class : char parameter

[Uros Bizjak]

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.