YunQiang Su <s...@gcc.gnu.org> writes:
> PR target/113179.
>
> In `store_bit_field_using_insv`, we just use SUBREG if value_mode
>>= op_mode, while in some ports, a sign_extend will be needed,
> such as MIPS64:
> If either GPR rs or GPR rt does not contain sign-extended 32-bit
> values (bits 63..31 equal), then the result of the operation is
> UNPREDICTABLE.
>
> The problem happens for the code like:
> struct xx {
> int a:4;
> int b:24;
> int c:3;
> int d:1;
> };
>
> void xx (struct xx *a, long long b) {
> a->d = b;
> }
>
> In the above code, the hard register contains `b`, may be note well
> sign-extended.
>
> gcc/
> PR target/113179
> * expmed.c(store_bit_field_using_insv): TRUNCATE value1 if
> needed.
>
> gcc/testsuite
> PR target/113179
> * gcc.target/mips/pr113179.c: New tests.
> ---
> gcc/expmed.cc | 12 +++++++++---
> gcc/testsuite/gcc.target/mips/pr113179.c | 18 ++++++++++++++++++
> 2 files changed, 27 insertions(+), 3 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/mips/pr113179.c
>
> diff --git a/gcc/expmed.cc b/gcc/expmed.cc
> index 4ec035e4843..6a582593da8 100644
> --- a/gcc/expmed.cc
> +++ b/gcc/expmed.cc
> @@ -704,9 +704,15 @@ store_bit_field_using_insv (const extraction_insn *insv,
> rtx op0,
> }
> else
> {
> - tmp = gen_lowpart_if_possible (op_mode, value1);
> - if (! tmp)
> - tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
> + if (targetm.mode_rep_extended (op_mode, value_mode))
> + tmp = simplify_gen_unary (TRUNCATE, op_mode,
> + value1, value_mode);
> + else
> + {
> + tmp = gen_lowpart_if_possible (op_mode, value1);
> + if (! tmp)
> + tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
> + }
> }
> value1 = tmp;
> }
I notice this patch is already applied. Was it approved? I didn't
see an approval in my feed or in the archives.
Although it might not make any difference on current targets,
I think the conditional should logically be based on
TRULY_NOOP_TRUNCATION(_MODES_P) rather than targetm.mode_rep_extended.
TRULY_NOOP_TRUNCATION is a correctness question: can I use subregs
to do this truncation? targetm.mode_rep_extended is instead an
optimisation question: can I assume that a particular extension is free?
Here we're trying to avoid a subreg for correctness, rather than trying
to take advantage of a cheap extension.
So I think the code should be:
if (GET_MODE_SIZE (value_mode) < GET_MODE_SIZE (op_mode))
{
tmp = simplify_subreg (op_mode, value1, value_mode, 0);
if (! tmp)
tmp = simplify_gen_subreg (op_mode,
force_reg (value_mode, value1),
value_mode, 0);
}
else if (GET_MODE_SIZE (op_mode) < GET_MODE_SIZE (value_mode)
&& !TRULY_NOOP_TRUNCATION_MODES_P (op_mode, value_mode))
tmp = simplify_gen_unary (TRUNCATE, op_mode, value1, value_mode);
else
{
tmp = gen_lowpart_if_possible (op_mode, value1);
if (! tmp)
tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
}
(also inclues unnesting of the else). Could you try changing the code
to do that and push the change if it works?
IMO the patch (in that form) is OK for backports after it has had a
couple of weeks on trunk.
Thanks,
Richard
> diff --git a/gcc/testsuite/gcc.target/mips/pr113179.c
> b/gcc/testsuite/gcc.target/mips/pr113179.c
> new file mode 100644
> index 00000000000..f32c5a16765
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/mips/pr113179.c
> @@ -0,0 +1,18 @@
> +/* Check if the operand of INS is sign-extended on MIPS64. */
> +/* { dg-options "-mips64r2 -mabi=64" } */
> +/* { dg-skip-if "code quality test" { *-*-* } { "-O0" } { "" } } */
> +
> +struct xx {
> + int a:1;
> + int b:24;
> + int c:6;
> + int d:1;
> +};
> +
> +long long xx (struct xx *a, long long b) {
> + a->d = b;
> + return b+1;
> +}
> +
> +/* { dg-final { scan-assembler "\tsll\t\\\$3,\\\$5,0" } } */
> +/* { dg-final { scan-assembler "\tdaddiu\t\\\$2,\\\$5,1" } } */
