Fix target/93119 (aarch64): ICE with traditional TLS support on ILP32
diff mbox series

Message ID 1579302817-19505-1-git-send-email-apinski@marvell.com
State New
Headers show
Series
  • Fix target/93119 (aarch64): ICE with traditional TLS support on ILP32
Related show

Commit Message

Andrew Pinski Jan. 17, 2020, 11:13 p.m. UTC
From: Andrew Pinski <apinski@marvell.com>

The problem here was g:23b88fda665d2f995c was not a complete fix
for supporting tranditional TLS on ILP32.

So the problem here is a couple of things, first __tls_get_addr
call will return a C pointer value so we need to use ptr_mode
when we are creating the call.  Then we need to convert
back that register to the correct mode, either zero extending
it or just creating a move instruction.
Also symbol_ref can either be in SImode or DImode.  So we need to
allow both modes.

Built and tested on aarch64-linux-gnu with no regressions.
Also built a full toolchain (including glibc) defaulting to traditional
TLS that targets ilp32 and lp64.

OK?

Thanks,
Andrew Pinski

ChangeLog:
* config/aarch64/aarch64.md (tlsgd_small_<mode>): Have operand 0 as PTR mode.
Have operand 1 as being modeless, it can be P mode.
(*tlsgd_small_<mode>): Likewise.
* config/aarch64/aarch64.c (aarch64_load_symref_appropriately)
<case SYMBOL_SMALL_TLSGD>: Call gen_tlsgd_small_* with a ptr_mode
register.  Convert that register back to dest using convert_mode.

Change-Id: I76826350d6bace0b731f21df1f125e9122da843f
---
 gcc/config/aarch64/aarch64.c               | 18 ++++++++++++++----
 gcc/config/aarch64/aarch64.md              |  8 ++++----
 gcc/testsuite/gcc.target/aarch64/pr93119.c | 10 ++++++++++
 3 files changed, 28 insertions(+), 8 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr93119.c

Comments

Richard Sandiford Jan. 20, 2020, 11:47 a.m. UTC | #1
<apinski@marvell.com> writes:
> From: Andrew Pinski <apinski@marvell.com>
>
> The problem here was g:23b88fda665d2f995c was not a complete fix
> for supporting tranditional TLS on ILP32.
>
> So the problem here is a couple of things, first __tls_get_addr
> call will return a C pointer value so we need to use ptr_mode
> when we are creating the call.  Then we need to convert
> back that register to the correct mode, either zero extending
> it or just creating a move instruction.

This and the comment:

> +	/* Convert back to the mode of the dest, adding a zero-extend as
> +	   needed. */
> +	if (dest != tmp_reg)
> +	  convert_move (dest, tmp_reg, true);

make it sound like the convert_move is handling two separate cases.
But AFAICT the convert_move only ever zero-extends, it should never
emit just a plain move.  Is that right?

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index c26ac0db942..6c825459dc6 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -2607,19 +2607,29 @@ aarch64_load_symref_appropriately (rtx dest, rtx imm,
>      case SYMBOL_SMALL_TLSGD:
>        {
>  	rtx_insn *insns;
> -	machine_mode mode = GET_MODE (dest);
> -	rtx result = gen_rtx_REG (mode, R0_REGNUM);
> +	/* The return type of __tls_get_addr is the C pointer type
> +	   so use ptr_mode.  */
> +	rtx result = gen_rtx_REG (ptr_mode, R0_REGNUM);
> +	rtx tmp_reg = dest;
> +
> +	if (GET_MODE (dest) != ptr_mode)
> +	  tmp_reg = can_create_pseudo_p () ? gen_reg_rtx (ptr_mode) : result;
>  
>  	start_sequence ();
> -	if (TARGET_ILP32)
> +	if (ptr_mode == SImode)
>  	  aarch64_emit_call_insn (gen_tlsgd_small_si (result, imm));
>  	else
>  	  aarch64_emit_call_insn (gen_tlsgd_small_di (result, imm));
> +
>  	insns = get_insns ();
>  	end_sequence ();

I think this looks more obvious without the extra blank line, so that
the whole start_sequence/end_sequence is a single block.

If the convert_move does always zero-extend, the patch is OK without the
extra blank line and with an updated comment.

Thanks,
Richard

>  
>  	RTL_CONST_CALL_P (insns) = 1;
> -	emit_libcall_block (insns, dest, result, imm);
> +	emit_libcall_block (insns, tmp_reg, result, imm);
> +	/* Convert back to the mode of the dest, adding a zero-extend as
> +	   needed. */
> +	if (dest != tmp_reg)
> +	  convert_move (dest, tmp_reg, true);
>  	return;
>        }
>  
> diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
> index 86c2cdfc797..55dde54b16a 100644
> --- a/gcc/config/aarch64/aarch64.md
> +++ b/gcc/config/aarch64/aarch64.md
> @@ -6755,10 +6755,10 @@ (define_insn "aarch64_load_tp_hard"
>  ;; instructions in the TLS stubs, in order to enable linker relaxation.
>  ;; Therefore we treat the stubs as an atomic sequence.
>  (define_expand "tlsgd_small_<mode>"
> - [(parallel [(set (match_operand 0 "register_operand")
> + [(parallel [(set (match_operand:PTR 0 "register_operand")
>                    (call (mem:DI (match_dup 2)) (const_int 1)))
>  	     (unspec:DI [(const_int 0)] UNSPEC_CALLEE_ABI)
> -	     (unspec:DI [(match_operand:PTR 1 "aarch64_valid_symref")] UNSPEC_GOTSMALLTLS)
> +	     (unspec:DI [(match_operand 1 "aarch64_valid_symref")] UNSPEC_GOTSMALLTLS)
>  	     (clobber (reg:DI LR_REGNUM))])]
>   ""
>  {
> @@ -6766,10 +6766,10 @@ (define_expand "tlsgd_small_<mode>"
>  })
>  
>  (define_insn "*tlsgd_small_<mode>"
> -  [(set (match_operand 0 "register_operand" "")
> +  [(set (match_operand:PTR 0 "register_operand" "")
>  	(call (mem:DI (match_operand:DI 2 "" "")) (const_int 1)))
>     (unspec:DI [(const_int 0)] UNSPEC_CALLEE_ABI)
> -   (unspec:DI [(match_operand:PTR 1 "aarch64_valid_symref" "S")] UNSPEC_GOTSMALLTLS)
> +   (unspec:DI [(match_operand 1 "aarch64_valid_symref" "S")] UNSPEC_GOTSMALLTLS)
>     (clobber (reg:DI LR_REGNUM))
>    ]
>    ""
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr93119.c b/gcc/testsuite/gcc.target/aarch64/pr93119.c
> new file mode 100644
> index 00000000000..93fa80e10b6
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr93119.c
> @@ -0,0 +1,10 @@
> +/* { dg-require-effective-target fpic } */
> +/* { dg-options "-mtls-dialect=trad -fpic" } */
> +
> +__thread int g_tlsdata;
> +
> +int func1()
> +{
> +  g_tlsdata++;
> +  return g_tlsdata;
> +}

Patch
diff mbox series

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index c26ac0db942..6c825459dc6 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -2607,19 +2607,29 @@  aarch64_load_symref_appropriately (rtx dest, rtx imm,
     case SYMBOL_SMALL_TLSGD:
       {
 	rtx_insn *insns;
-	machine_mode mode = GET_MODE (dest);
-	rtx result = gen_rtx_REG (mode, R0_REGNUM);
+	/* The return type of __tls_get_addr is the C pointer type
+	   so use ptr_mode.  */
+	rtx result = gen_rtx_REG (ptr_mode, R0_REGNUM);
+	rtx tmp_reg = dest;
+
+	if (GET_MODE (dest) != ptr_mode)
+	  tmp_reg = can_create_pseudo_p () ? gen_reg_rtx (ptr_mode) : result;
 
 	start_sequence ();
-	if (TARGET_ILP32)
+	if (ptr_mode == SImode)
 	  aarch64_emit_call_insn (gen_tlsgd_small_si (result, imm));
 	else
 	  aarch64_emit_call_insn (gen_tlsgd_small_di (result, imm));
+
 	insns = get_insns ();
 	end_sequence ();
 
 	RTL_CONST_CALL_P (insns) = 1;
-	emit_libcall_block (insns, dest, result, imm);
+	emit_libcall_block (insns, tmp_reg, result, imm);
+	/* Convert back to the mode of the dest, adding a zero-extend as
+	   needed. */
+	if (dest != tmp_reg)
+	  convert_move (dest, tmp_reg, true);
 	return;
       }
 
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 86c2cdfc797..55dde54b16a 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -6755,10 +6755,10 @@  (define_insn "aarch64_load_tp_hard"
 ;; instructions in the TLS stubs, in order to enable linker relaxation.
 ;; Therefore we treat the stubs as an atomic sequence.
 (define_expand "tlsgd_small_<mode>"
- [(parallel [(set (match_operand 0 "register_operand")
+ [(parallel [(set (match_operand:PTR 0 "register_operand")
                   (call (mem:DI (match_dup 2)) (const_int 1)))
 	     (unspec:DI [(const_int 0)] UNSPEC_CALLEE_ABI)
-	     (unspec:DI [(match_operand:PTR 1 "aarch64_valid_symref")] UNSPEC_GOTSMALLTLS)
+	     (unspec:DI [(match_operand 1 "aarch64_valid_symref")] UNSPEC_GOTSMALLTLS)
 	     (clobber (reg:DI LR_REGNUM))])]
  ""
 {
@@ -6766,10 +6766,10 @@  (define_expand "tlsgd_small_<mode>"
 })
 
 (define_insn "*tlsgd_small_<mode>"
-  [(set (match_operand 0 "register_operand" "")
+  [(set (match_operand:PTR 0 "register_operand" "")
 	(call (mem:DI (match_operand:DI 2 "" "")) (const_int 1)))
    (unspec:DI [(const_int 0)] UNSPEC_CALLEE_ABI)
-   (unspec:DI [(match_operand:PTR 1 "aarch64_valid_symref" "S")] UNSPEC_GOTSMALLTLS)
+   (unspec:DI [(match_operand 1 "aarch64_valid_symref" "S")] UNSPEC_GOTSMALLTLS)
    (clobber (reg:DI LR_REGNUM))
   ]
   ""
diff --git a/gcc/testsuite/gcc.target/aarch64/pr93119.c b/gcc/testsuite/gcc.target/aarch64/pr93119.c
new file mode 100644
index 00000000000..93fa80e10b6
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr93119.c
@@ -0,0 +1,10 @@ 
+/* { dg-require-effective-target fpic } */
+/* { dg-options "-mtls-dialect=trad -fpic" } */
+
+__thread int g_tlsdata;
+
+int func1()
+{
+  g_tlsdata++;
+  return g_tlsdata;
+}