Thank you for your suggestions. Looks like I have pasted the wrong test case.
I'm sorry for that.
I have modified accordingly and changed to use the correct test now in my new
patch.
I have carried a full test, no new failure witnessed. Newly added test fail
without the patch and pass after applying the patch.
Attached please find the my new patch. Can you sponsor it if it's OK to go?
Thanks,
Duan bo
-邮件原件-
发件人: Richard Sandiford [mailto:richard.sandif...@arm.com]
发送时间: 2020年3月17日 18:30
收件人: duanbo (C)
抄送: gcc-patches@gcc.gnu.org
主题: Re: [PATCH PR94201] aarch64：ICE in tiny code model for ilp32
"duanbo (C)" 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_"
Making this "@ldr_got_tiny_" 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%0, %L1"
Pre-existing, not your fault, but: there only needs to be a single backslash
here.
> + [(set_attr "type" "load_")]
> +)
> +
> +(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
