"duanbo (C)" <duan...@huawei.com> writes:
> Hi
> The testcase pr78255.c triggers ice when testing GCC trunk on aarch64 with
> -mabi=ilp32 -fPIC.
> Case path: gcc/testsuite/gcc.target/aarch64/pr78255.c
>
> The ICE is caused by insufficient support for the tiny code model under ilp32
> mode with fPIC option, where a SI mode register might be used for the ldr
> instruction.
> However, current md pattern for UNSPEC_GOTTINYPIC only support DI mode
> register as its operator.
>
> A simple solution is to add the pattern in tiny code model to support ilp32
> mode.
> Attached please find the proposed patch.
> Newly add test fail without the patch and pass after applying the patch.
> Bootstrap and tested on aarch64 Linux platform. No new regression witnessed.
> Any suggestion?
Thanks for doing this. The patch looks good, but some minor comments below.
However, the test fails for me on aarch64-linux-gnu:
FAIL: gcc.target/aarch64/pr94201.c scan-assembler ldr w0, \\s+a
FAIL: gcc.target/aarch64/pr94201.c scan-assembler b\\s+bar
Could you double-check your set-up to make sure that it's running the
testsuite correctly? With scan-assembler tests, it can be useful to
introduce a deliberate typo and check that the test fails, then remove
the typo and check that the test passes.
You probably already know, but it's possible to run just the new
test by adding:
RUNTESTFLAGS=aarch64.exp=pr94201.c
to the "make check-gcc" command line. The final patch still needs to be
tested against the full testsuite of course, but using RUNTESTFLAGS can
save time while developing the patch.
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index b0cbb6e2d55..c56ed733e19 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -2739,8 +2739,26 @@ aarch64_load_symref_appropriately (rtx dest, rtx imm,
> }
>
> case SYMBOL_TINY_GOT:
> - emit_insn (gen_ldr_got_tiny (dest, imm));
> - return;
> + {
> + rtx insn;
> + machine_mode mode = GET_MODE (dest);
> +
> + if (mode == ptr_mode)
> + {
> + if (mode == DImode)
> + insn = gen_ldr_got_tiny_di (dest, imm);
> + else
> + insn = gen_ldr_got_tiny_si (dest, imm);
> + }
With the .md change mentioned below, we can instead use:
if (mode == ptr_mode)
insn = gen_ldr_got_tiny (mode, dest, imm);
> + else
> + {
> + gcc_assert (mode == Pmode);
> + insn = gen_ldr_got_tiny_sidi (dest, imm);
> + }
> +
> + emit_insn (insn);
> + return;
> + }
>
> case SYMBOL_TINY_TLSIE:
> {
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 7ad4e918578..debc6656670 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -6766,13 +6766,23 @@
> [(set_attr "type" "load_4")]
> )
>
> -(define_insn "ldr_got_tiny"
> - [(set (match_operand:DI 0 "register_operand" "=r")
> - (unspec:DI [(match_operand:DI 1 "aarch64_valid_symref" "S")]
> - UNSPEC_GOTTINYPIC))]
> +(define_insn "ldr_got_tiny_<mode>"
Making this "@ldr_got_tiny_<mode>" creates a gen_ldr_got_tiny
function that takes the mode as the first argument. We can then
use this function in the aarch64.c code above.
> + [(set (match_operand:PTR 0 "register_operand" "=r")
> + (unspec:PTR [(match_operand:PTR 1 "aarch64_valid_symref" "S")]
> + UNSPEC_GOTTINYPIC))]
Very minor formatting issue, but: the usual style in the aarch64 port
is to indent the unspec name to the same column as the "[", like the
original pattern did.
> ""
> - "ldr\\t%0, %L1"
> - [(set_attr "type" "load_8")]
> + "ldr\\t%<w>0, %L1"
Pre-existing, not your fault, but: there only needs to be a single
backslash here.
> + [(set_attr "type" "load_<ldst_sz>")]
> +)
> +
> +(define_insn "ldr_got_tiny_sidi"
> + [(set (match_operand:DI 0 "register_operand" "=r")
> + (zero_extend:DI
> + (unspec:SI [(match_operand:DI 1 "aarch64_valid_symref" "S")]
> + UNSPEC_GOTTINYPIC)))]
Same unspec formatting comment as above. Also, the usual style
in the aarch64 port is to indent the operand of something like
(zero_extend by two extra spaces, giving:
(define_insn "ldr_got_tiny_sidi"
[(set (match_operand:DI 0 "register_operand" "=r")
(zero_extend:DI
(unspec:SI [(match_operand:DI 1 "aarch64_valid_symref" "S")]
UNSPEC_GOTTINYPIC)))]
(but with 8 spaces converted to tabs).
> + "TARGET_ILP32"
> + "ldr\\t%w0, %L1"
We only need a single backslash here too.
Thanks,
Richard
> + [(set_attr "type" "load_4")]
> )
>
> (define_insn "aarch64_load_tp_hard"
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr94201.c
> b/gcc/testsuite/gcc.target/aarch64/pr94201.c
> new file mode 100644
> index 00000000000..f822ddea7bd
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr94201.c
> @@ -0,0 +1,15 @@
> +/* { dg-do compile } */
> +/* { dg-options "-mcmodel=tiny -mabi=ilp32 -fPIC" } */
> +
> +extern int bar (void *);
> +extern long long a;
> +
> +int
> +foo (void)
> +{
> + a=1;
> + return bar ((void *)bar);
> +}
> +
> +/* { dg-final { scan-assembler "ldr w0, \\s+a" } } */
> +/* { dg-final { scan-assembler "b\\s+bar" } } */
