ping
From: Wilco Dijkstra
Sent: 13 February 2019 12:23
To: Ramana Radhakrishnan
Cc: GCC Patches; nd; Olivier Hainque
Subject: Re: [PATCH][ARM] Fix PR89222
Hi Ramana,
ARMv5te bootstrap OK, regression tests pass. OK for commit?
Interesting bug. armv5te-linux bootstrap ? Can you share your --target
and --with-arch flags ?
--target/host/build=arm-linux-gnueabi --with-arch=armv5te --with-mode=arm
+ if (GET_CODE (base) == SYMBOL_REF)
Isn't there a SYMBOL_REF_P predicate for this ?
Yes, I've changed this one, but this really should be done as a cleanup across
Arm and AArch64 given there are 100 occurrences that need to be fixed.
Can we look to allow anything that is a power of 2 as an offset i.e.
anything with bit 0 set to 0 ? Could you please file an enhancement
request on binutils for both gold and ld to catch the linker warning
case ? I suspect we are looking for addends which have the lower bit
set and function symbols ?
I don't think it is useful optimization to allow an offset on function symbols.
A linker warning would be useful indeed, I'll file an enhancement request.
Firstly (targetm.cannot_force_const_mem (...)) please instead of
arm_cannot_force_const_mem , then that can remain static. Let's look
to use the targetm interface instead of direct calls here. We weren't
I've changed it to use targetm in the new version.
hitting this path for non-vxworks code , however now we do so if
arm_tls_referenced_p is true at the end of arm_cannot_force_const_mem
which means that we could well have a TLS address getting spat out or
am I mis-reading something ?
Yes there was a path where we could end up in an endless expansion loop
if arm_tls_referenced_p is true. I fixed this by checking the offset is nonzero
before expanding. This also allowed a major simplification of the TLS code
which was trying to do the same thing.
Can Olivier or someone who cares about vxworks also give this a quick
sanity run for the alternate code path once we resolve some of the
review questions here ? Don't we also need to worry about
-mslow-flash-data where we get rid of literal pools and have movw /
movt instructions ?
Splitting the offset early means it works fine for MOVW/MOVT. Eg before
my change -mcpu=cortex-m3 -mslow-flash-data:
f3:
movw r3, #:lower16:handler-1
movt r3, #:upper16:handler-1
After:
movw r3, #:lower16:handler
movt r3, #:upper16:handler
subs r3, r3, #1
Here is the updated version:
The GCC optimizer can generate symbols with non-zero offset from simple
if-statements. Bit zero is used for the Arm/Thumb state bit, so relocations
with offsets fail if it changes bit zero and the relocation forces bit zero
to true. The fix is to disable offsets on function pointer symbols.
ARMv5te and armhf bootstrap OK, regression tests pass. OK for commit?
ChangeLog:
2019-02-12 Wilco Dijkstra <wdijk...@arm.com>
gcc/
PR target/89222
* config/arm/arm.md (movsi): Use targetm.cannot_force_const_mem
to decide when to split off a non-zero offset from a symbol.
* config/arm/arm.c (arm_cannot_force_const_mem): Disallow offsets
in function symbols.
testsuite/
PR target/89222
* gcc.target/arm/pr89222.c: Add new test.
---
diff --git a/gcc/config/arm/arm.c b/gcc/config/arm/arm.c
index
f07f4cc47b6cfcea8f44960bf4760ea9a46b8f87..69b74a237a5f10b4137aa995ad43b77d6ecd04db
100644
--- a/gcc/config/arm/arm.c
+++ b/gcc/config/arm/arm.c
@@ -8940,11 +8940,16 @@ static bool
arm_cannot_force_const_mem (machine_mode mode ATTRIBUTE_UNUSED, rtx x)
{
rtx base, offset;
+ split_const (x, &base, &offset);
- if (ARM_OFFSETS_MUST_BE_WITHIN_SECTIONS_P)
+ if (SYMBOL_REF_P (base))
{
- split_const (x, &base, &offset);
- if (GET_CODE (base) == SYMBOL_REF
+ /* Function symbols cannot have an offset due to the Thumb bit. */
+ if ((SYMBOL_REF_FLAGS (base) & SYMBOL_FLAG_FUNCTION)
+ && INTVAL (offset) != 0)
+ return true;
+
+ if (ARM_OFFSETS_MUST_BE_WITHIN_SECTIONS_P
&& !offset_within_block_p (base, INTVAL (offset)))
return true;
}
diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index
aa759624f8f617576773aa75fd6239d6e06e8a13..88110cb732b52866d4fdcad70fd5a202aa62bd03
100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -5981,53 +5981,29 @@ (define_expand "movsi"
}
}
- if (ARM_OFFSETS_MUST_BE_WITHIN_SECTIONS_P)
+ split_const (operands[1], &base, &offset);
+ if (INTVAL (offset) != 0
+ && targetm.cannot_force_const_mem (SImode, operands[1]))
{
- split_const (operands[1], &base, &offset);
- if (GET_CODE (base) == SYMBOL_REF
- && !offset_within_block_p (base, INTVAL (offset)))
- {
- tmp = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
- emit_move_insn (tmp, base);
- emit_insn (gen_addsi3 (operands[0], tmp, offset));
- DONE;
- }
+ tmp = can_create_pseudo_p () ? gen_reg_rtx (SImode) : operands[0];
+ emit_move_insn (tmp, base);
+ emit_insn (gen_addsi3 (operands[0], tmp, offset));
+ DONE;
}
+ tmp = can_create_pseudo_p () ? NULL_RTX : operands[0];
+
/* Recognize the case where operand[1] is a reference to thread-local
- data and load its address to a register. */
+ data and load its address to a register. Offsets have been split off
+ already. */
if (arm_tls_referenced_p (operands[1]))
- {
- rtx tmp = operands[1];
- rtx addend = NULL;
-
- if (GET_CODE (tmp) == CONST && GET_CODE (XEXP (tmp, 0)) == PLUS)
- {
- addend = XEXP (XEXP (tmp, 0), 1);
- tmp = XEXP (XEXP (tmp, 0), 0);
- }
-
- gcc_assert (GET_CODE (tmp) == SYMBOL_REF);
- gcc_assert (SYMBOL_REF_TLS_MODEL (tmp) != 0);
-
- tmp = legitimize_tls_address (tmp,
- !can_create_pseudo_p () ? operands[0] : 0);
- if (addend)
- {
- tmp = gen_rtx_PLUS (SImode, tmp, addend);
- tmp = force_operand (tmp, operands[0]);
- }
- operands[1] = tmp;
- }
+ operands[1] = legitimize_tls_address (operands[1], tmp);
else if (flag_pic
&& (CONSTANT_P (operands[1])
|| symbol_mentioned_p (operands[1])
|| label_mentioned_p (operands[1])))
- operands[1] = legitimize_pic_address (operands[1], SImode,
- (!can_create_pseudo_p ()
- ? operands[0]
- : NULL_RTX), NULL_RTX,
- false /*compute_now*/);
+ operands[1] =
+ legitimize_pic_address (operands[1], SImode, tmp, NULL_RTX, false);
}
"
)
diff --git a/gcc/testsuite/gcc.target/arm/pr89222.c
b/gcc/testsuite/gcc.target/arm/pr89222.c
new file mode 100644
index
0000000000000000000000000000000000000000..d26d7df17544db8426331e67b9a36d749ec6c6d1
--- /dev/null
+++ b/gcc/testsuite/gcc.target/arm/pr89222.c
@@ -0,0 +1,32 @@
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+void g (void);
+
+void f1 (int x)
+{
+ if (x != (int) g + 3)
+ return;
+ g();
+}
+
+void (*a2)(void);
+
+void f2 (void)
+{
+ a2 = &g + 3;
+}
+
+typedef void (*__sighandler_t)(int);
+void handler (int);
+
+void f3 (int x)
+{
+ __sighandler_t h = &handler;
+ if (h != (__sighandler_t) 2 && h != (__sighandler_t) 1)
+ h (x);
+}
+
+/* { dg-final { scan-assembler-times {add(?:s)?\tr[0-9]+, r[0-9]+, #3} 2 } } */
+/* { dg-final { scan-assembler-not {.word\tg\+3} } } */
+/* { dg-final { scan-assembler-not {.word\thandler-1} } } */
