Re: [PATCH][AArch64] Mark symbols as constant

[Wilco Dijkstra]

Andreas Schwab wrote:
>
> This breaks gcc.target/aarch64/reload-valid-spoff.c with -mabi=ilp32:

Indeed, there is a odd ILP32 bug that causes high/lo_sum to be generated
in SI mode in expand:

(insn 15 14 16 4 (set (reg:SI 125)
(high:SI (symbol_ref/u:DI ("*.LC1") [flags 0x2]))) 
 (nil))
(insn 16 15 17 4 (set (reg:SI 124)
(lo_sum:SI (reg:SI 125)
(symbol_ref/u:DI ("*.LC1") [flags 0x2])))

Eventually this may end up as a 64-bit adrp, a 32-bit lo_sum and a load that
expects a 64-bit address...

I have a simple workaround to disable the symbol optimization in ILP32,
but I'm looking into fixing the underlying cause.

Wilco

Re: [PATCH][AArch64] Mark symbols as constant

[Andreas Schwab]

On Jun 20 2017, Wilco Dijkstra  wrote:

>   * config/aarch64/aarch64.c (aarch64_legitimate_constant_p):
>   Return true for non-tls symbols.

This breaks gcc.target/aarch64/reload-valid-spoff.c with -mabi=ilp32:

/opt/gcc/gcc-20170622/gcc/testsuite/gcc.target/aarch64/reload-valid-spoff.c: In 
function 'arp_file':
/opt/gcc/gcc-20170622/gcc/testsuite/gcc.target/aarch64/reload-valid-spoff.c:63:1:
 internal compiler error: in plus_constant, at explow.c:88
0x801ebf plus_constant(machine_mode, rtx_def*, long, bool)
../../gcc/explow.c:88
0x10b66df recog_126
../../gcc/config/aarch64/aarch64.md:1188
0x10b66df recog_128
../../gcc/config/aarch64/aarch64.md:2685
0x10b66df recog_129
../../gcc/config/aarch64/aarch64-simd.md:4348
0x10e851f recog_for_combine_1
../../gcc/combine.c:11148
0x10e8ba7 recog_for_combine
../../gcc/combine.c:11404
0x10f8ca7 try_combine
../../gcc/combine.c:3520
0x10fbcbb combine_instructions
../../gcc/combine.c:1275
0x10fbcbb rest_of_handle_combine
../../gcc/combine.c:14653
0x10fbcbb execute
../../gcc/combine.c:14698

Andreas.

-- 
Andreas Schwab, SUSE Labs, sch...@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH][AArch64] Mark symbols as constant

[Richard Earnshaw (lists)]

On 20/06/17 15:56, Wilco Dijkstra wrote:
> Richard Earnshaw wrote:
> 
>> What testing has this had with -fpic?  I'm not convinced that this
>> assertion is true in that case?
> 
> I ran the GLIBC tests which pass. -fpic works since it does also form a
> constant address, ie. instead of:
> 
> adrp  x1, global
> add x1, x1, :lo12:global
> 
> we do:
> 
> adrp  x1, :got:global
> ldr x1, [x1, :got_lo12:global]
> 
> CSEing or rematerializing either sequence works in the same way.
> 
> With TLS the resulting addresses are also constant, however this could
> cause rather complex TLS sequences to be rematerialized.  It seems
> best to block that.  Updated patch below:
> 
> 
> Aarch64_legitimate_constant_p currently returns false for symbols,
> eventhough they are always valid constants.  This means LOSYM isn't
> CSEd correctly.  If we return true CSE works better, resulting in
> smaller/faster code (0.3% smaller code on SPEC2006).  Avoid this
> for TLS symbols since their sequence is complex.
> 
> int x0 = 1, x1 = 2, x2 = 3;
> 
> int 
> f (int x, int y)
> {
>   x += x1;
>   if (x > 100)
> y += x2;
>   x += x0;
>   return x + y;
> }
> 
> Before:
>   adrpx3, .LANCHOR0
>   add x4, x3, :lo12:.LANCHOR0
>   ldr w2, [x3, #:lo12:.LANCHOR0]
>   add w0, w0, w2
>   cmp w0, 100
>   ble .L5
>   ldr w2, [x4, 8]
>   add w1, w1, w2
> .L5:
>   add x3, x3, :lo12:.LANCHOR0
>   ldr w2, [x3, 4]
>   add w0, w0, w2
>   add w0, w0, w1
>   ret
> 
> After:
>   adrpx2, .LANCHOR0
>   add x3, x2, :lo12:.LANCHOR0
>   ldr w2, [x2, #:lo12:.LANCHOR0]
>   add w0, w0, w2
>   cmp w0, 100
>   ble .L5
>   ldr w2, [x3, 8]
>   add w1, w1, w2
> .L5:
>   ldr w2, [x3, 4]
>   add w0, w0, w2
>   add w0, w0, w1
>   ret
> 
> Bootstrap OK, OK for commit?
> 
> ChangeLog:
> 2017-06-20  Wilco Dijkstra  
> 
>   * config/aarch64/aarch64.c (aarch64_legitimate_constant_p):
>   Return true for non-tls symbols.
> --

OK.

R.

> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 
> 5ec6bbfcf484baa4005b8a88cb98d0d04f710877..060cd8476d2954119daac495ecb059c9be73edbe
>  100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -10111,6 +10111,11 @@ aarch64_legitimate_constant_p (machine_mode mode, 
> rtx x)
>&& aarch64_valid_symref (XEXP (x, 0), GET_MODE (XEXP (x, 0
>  return true;
>  
> +  /* Treat symbols as constants.  Avoid TLS symbols as they are complex,
> + so spilling them is better than rematerialization.  */
> +  if (SYMBOL_REF_P (x) && !SYMBOL_REF_TLS_MODEL (x))
> +return true;
> +
>return aarch64_constant_address_p (x);
>  }
>  
> 

Re: [PATCH][AArch64] Mark symbols as constant

[Wilco Dijkstra]

Richard Earnshaw wrote:

> What testing has this had with -fpic?  I'm not convinced that this
> assertion is true in that case?

I ran the GLIBC tests which pass. -fpic works since it does also form a
constant address, ie. instead of:

adrp  x1, global
add x1, x1, :lo12:global

we do:

adrp  x1, :got:global
ldr x1, [x1, :got_lo12:global]

CSEing or rematerializing either sequence works in the same way.

With TLS the resulting addresses are also constant, however this could
cause rather complex TLS sequences to be rematerialized.  It seems
best to block that.  Updated patch below:


Aarch64_legitimate_constant_p currently returns false for symbols,
eventhough they are always valid constants.  This means LOSYM isn't
CSEd correctly.  If we return true CSE works better, resulting in
smaller/faster code (0.3% smaller code on SPEC2006).  Avoid this
for TLS symbols since their sequence is complex.

int x0 = 1, x1 = 2, x2 = 3;

int 
f (int x, int y)
{
  x += x1;
  if (x > 100)
y += x2;
  x += x0;
  return x + y;
}

Before:
adrpx3, .LANCHOR0
add x4, x3, :lo12:.LANCHOR0
ldr w2, [x3, #:lo12:.LANCHOR0]
add w0, w0, w2
cmp w0, 100
ble .L5
ldr w2, [x4, 8]
add w1, w1, w2
.L5:
add x3, x3, :lo12:.LANCHOR0
ldr w2, [x3, 4]
add w0, w0, w2
add w0, w0, w1
ret

After:
adrpx2, .LANCHOR0
add x3, x2, :lo12:.LANCHOR0
ldr w2, [x2, #:lo12:.LANCHOR0]
add w0, w0, w2
cmp w0, 100
ble .L5
ldr w2, [x3, 8]
add w1, w1, w2
.L5:
ldr w2, [x3, 4]
add w0, w0, w2
add w0, w0, w1
ret

Bootstrap OK, OK for commit?

ChangeLog:
2017-06-20  Wilco Dijkstra  

* config/aarch64/aarch64.c (aarch64_legitimate_constant_p):
Return true for non-tls symbols.
--
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 
5ec6bbfcf484baa4005b8a88cb98d0d04f710877..060cd8476d2954119daac495ecb059c9be73edbe
 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10111,6 +10111,11 @@ aarch64_legitimate_constant_p (machine_mode mode, rtx 
x)
   && aarch64_valid_symref (XEXP (x, 0), GET_MODE (XEXP (x, 0
 return true;
 
+  /* Treat symbols as constants.  Avoid TLS symbols as they are complex,
+ so spilling them is better than rematerialization.  */
+  if (SYMBOL_REF_P (x) && !SYMBOL_REF_TLS_MODEL (x))
+return true;
+
   return aarch64_constant_address_p (x);
 }
 

Re: [PATCH][AArch64] Mark symbols as constant

[Richard Earnshaw]

On 19/06/17 19:59, Wilco Dijkstra wrote:
> Aarch64_legitimate_constant_p currently returns false for symbols,
> eventhough they are always valid constants.  This means LOSYM isn't
> CSEd correctly.  If we return true CSE works better, resulting in
> smaller/faster code (0.3% smaller code on SPEC2006).
> 
> int x0 = 1, x1 = 2, x2 = 3;
> 
> int 
> f (int x, int y)
> {
>   x += x1;
>   if (x > 100)
> y += x2;
>   x += x0;
>   return x + y;
> }
> 
> Before:
>   adrpx3, .LANCHOR0
>   add x4, x3, :lo12:.LANCHOR0
>   ldr w2, [x3, #:lo12:.LANCHOR0]
>   add w0, w0, w2
>   cmp w0, 100
>   ble .L5
>   ldr w2, [x4, 8]
>   add w1, w1, w2
> .L5:
>   add x3, x3, :lo12:.LANCHOR0
>   ldr w2, [x3, 4]
>   add w0, w0, w2
>   add w0, w0, w1
>   ret
> 
> After:
>   adrpx2, .LANCHOR0
>   add x3, x2, :lo12:.LANCHOR0
>   ldr w2, [x2, #:lo12:.LANCHOR0]
>   add w0, w0, w2
>   cmp w0, 100
>   ble .L5
>   ldr w2, [x3, 8]
>   add w1, w1, w2
> .L5:
>   ldr w2, [x3, 4]
>   add w0, w0, w2
>   add w0, w0, w1
>   ret
> 
> Passes regress and bootstrap, OK for commit?
> 
> ChangeLog:
> 2017-06-19  Wilco Dijkstra  
> 
>   * config/aarch64/aarch64.c (aarch64_legitimate_constant_p):
>   Return true for symbols.
> --
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 
> 5ec6bbfcf484baa4005b8a88cb98d0d04f710877..4b7d961102e41ce927d89d458fc89ddfc2adcd6f
>  100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -10111,6 +10111,9 @@ aarch64_legitimate_constant_p (machine_mode mode, rtx 
> x)
>&& aarch64_valid_symref (XEXP (x, 0), GET_MODE (XEXP (x, 0
>  return true;
>  
> +  if (SYMBOL_REF_P (x))
> +return true;
> +
>return aarch64_constant_address_p (x);
>  }
>  
> 

What testing has this had with -fpic?  I'm not convinced that this
assertion is true in that case?

R.

[PATCH][AArch64] Mark symbols as constant

[Wilco Dijkstra]

Aarch64_legitimate_constant_p currently returns false for symbols,
eventhough they are always valid constants.  This means LOSYM isn't
CSEd correctly.  If we return true CSE works better, resulting in
smaller/faster code (0.3% smaller code on SPEC2006).

int x0 = 1, x1 = 2, x2 = 3;

int 
f (int x, int y)
{
  x += x1;
  if (x > 100)
y += x2;
  x += x0;
  return x + y;
}

Before:
adrpx3, .LANCHOR0
add x4, x3, :lo12:.LANCHOR0
ldr w2, [x3, #:lo12:.LANCHOR0]
add w0, w0, w2
cmp w0, 100
ble .L5
ldr w2, [x4, 8]
add w1, w1, w2
.L5:
add x3, x3, :lo12:.LANCHOR0
ldr w2, [x3, 4]
add w0, w0, w2
add w0, w0, w1
ret

After:
adrpx2, .LANCHOR0
add x3, x2, :lo12:.LANCHOR0
ldr w2, [x2, #:lo12:.LANCHOR0]
add w0, w0, w2
cmp w0, 100
ble .L5
ldr w2, [x3, 8]
add w1, w1, w2
.L5:
ldr w2, [x3, 4]
add w0, w0, w2
add w0, w0, w1
ret

Passes regress and bootstrap, OK for commit?

ChangeLog:
2017-06-19  Wilco Dijkstra  

* config/aarch64/aarch64.c (aarch64_legitimate_constant_p):
Return true for symbols.
--
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 
5ec6bbfcf484baa4005b8a88cb98d0d04f710877..4b7d961102e41ce927d89d458fc89ddfc2adcd6f
 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -10111,6 +10111,9 @@ aarch64_legitimate_constant_p (machine_mode mode, rtx x)
   && aarch64_valid_symref (XEXP (x, 0), GET_MODE (XEXP (x, 0
 return true;
 
+  if (SYMBOL_REF_P (x))
+return true;
+
   return aarch64_constant_address_p (x);
 }