* [PATCH][AArch64] Mark symbols as constant
@ 2017-06-19 18:59 Wilco Dijkstra
2017-06-19 22:13 ` Richard Earnshaw
0 siblings, 1 reply; 6+ messages in thread
From: Wilco Dijkstra @ 2017-06-19 18:59 UTC (permalink / raw)
To: GCC Patches; +Cc: nd, James Greenhalgh
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:
adrp x3, .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:
adrp x2, .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 <wdijkstr@arm.com>
* 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);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Mark symbols as constant
2017-06-19 18:59 [PATCH][AArch64] Mark symbols as constant Wilco Dijkstra
@ 2017-06-19 22:13 ` Richard Earnshaw
2017-06-20 14:56 ` Wilco Dijkstra
0 siblings, 1 reply; 6+ messages in thread
From: Richard Earnshaw @ 2017-06-19 22:13 UTC (permalink / raw)
To: Wilco Dijkstra, GCC Patches; +Cc: nd, James Greenhalgh
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:
> adrp x3, .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:
> adrp x2, .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 <wdijkstr@arm.com>
>
> * 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Mark symbols as constant
2017-06-19 22:13 ` Richard Earnshaw
@ 2017-06-20 14:56 ` Wilco Dijkstra
2017-06-21 10:01 ` Richard Earnshaw (lists)
2017-06-22 15:22 ` Andreas Schwab
0 siblings, 2 replies; 6+ messages in thread
From: Wilco Dijkstra @ 2017-06-20 14:56 UTC (permalink / raw)
To: Richard Earnshaw, GCC Patches; +Cc: nd, James Greenhalgh
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:
adrp x3, .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:
adrp x2, .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 <wdijkstr@arm.com>
* 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);
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Mark symbols as constant
2017-06-20 14:56 ` Wilco Dijkstra
@ 2017-06-21 10:01 ` Richard Earnshaw (lists)
2017-06-22 15:22 ` Andreas Schwab
1 sibling, 0 replies; 6+ messages in thread
From: Richard Earnshaw (lists) @ 2017-06-21 10:01 UTC (permalink / raw)
To: Wilco Dijkstra, GCC Patches; +Cc: nd, James Greenhalgh
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:
> adrp x3, .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:
> adrp x2, .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 <wdijkstr@arm.com>
>
> * 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);
> }
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Mark symbols as constant
2017-06-20 14:56 ` Wilco Dijkstra
2017-06-21 10:01 ` Richard Earnshaw (lists)
@ 2017-06-22 15:22 ` Andreas Schwab
2017-06-23 10:56 ` Wilco Dijkstra
1 sibling, 1 reply; 6+ messages in thread
From: Andreas Schwab @ 2017-06-22 15:22 UTC (permalink / raw)
To: Wilco Dijkstra; +Cc: Richard Earnshaw, GCC Patches, nd, James Greenhalgh
On Jun 20 2017, Wilco Dijkstra <Wilco.Dijkstra@arm.com> 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, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE 1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH][AArch64] Mark symbols as constant
2017-06-22 15:22 ` Andreas Schwab
@ 2017-06-23 10:56 ` Wilco Dijkstra
0 siblings, 0 replies; 6+ messages in thread
From: Wilco Dijkstra @ 2017-06-23 10:56 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Richard Earnshaw, GCC Patches, nd, James Greenhalgh
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-06-23 10:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-19 18:59 [PATCH][AArch64] Mark symbols as constant Wilco Dijkstra
2017-06-19 22:13 ` Richard Earnshaw
2017-06-20 14:56 ` Wilco Dijkstra
2017-06-21 10:01 ` Richard Earnshaw (lists)
2017-06-22 15:22 ` Andreas Schwab
2017-06-23 10:56 ` Wilco Dijkstra
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).