Re: [PATCH][AArch64][GCC 6] PR target/79041: Correct -mpc-relative-literal-loads logic in aarch64_classify_symbol

[Yvan Roux <yvan dot roux at linaro dot org>]

Hi all,

On 16 January 2017 at 16:34, Kyrill Tkachov <kyrylo.tkachov@foss.arm.com> wrote:
>
> On 13/01/17 16:35, James Greenhalgh wrote:
>>
>> On Wed, Jan 11, 2017 at 04:32:45PM +0000, Kyrill Tkachov wrote:
>>>
>>> Hi all,
>>>
>>> In this PR we generated ADRP/ADD instructions with :lo12: relocations on
>>> symbols even though -mpc-relative-literal-loads is used. This is due to
>>> the
>>> confusing double-negative logic of the nopcrelative_literal_loads aarch64
>>> variable and its relation to the aarch64_nopcrelative_literal_loads
>>> global
>>> variable in the GCC 6 branch.
>>>
>>> Wilco fixed this on trunk as part of a larger patch (r237607) and parts
>>> of
>>> that patch were backported, but other parts weren't and that patch now
>>> doesn't apply cleanly to the branch.
>>
>> As I commented to Jakub at the time he made the first partial backport,
>> I'd much rather just see all of Wilco's patch backported. We're not on
>> the verge of a 6 release now, so please just backport Wilco's patch (as
>> should have been done all along if this had been correctly identified as
>> a fix rather than a clean-up).
>
>
> Unfortunately simply backporting the patch does not fix this PR.
> The aarch64_classify_symbol changes do not have the desired effect
> and the :lo12: relocations are generated.
> I'll look into it, but I believe that would require a bigger change than
> this one-liner.

Here is the backport of Wilco's patch (r237607) along with Kyrill's
one (r244643, which removed the remaining occurences of
aarch64_nopcrelative_literal_loads).  To fix the issue the original
patch has to be modified, to keep aarch64_pcrelative_literal_loads
test for large models in aarch64_classify_symbol.

On trunk and gcc-7-branch the :lo12: relocations are not generated
because of Wilco's fix for pr78733 (r243456 and 243486), but my
understanding is that the bug is still present since compiling
gcc.target/aarch64/pr78733.c with -mcmodel=large brings back the
:lo12: relocations (I'll submit a patch to add the test back if my
understanding is correct).

Cross built and regtested without issue for aarch64-linux-gnu,
aarch64-none-elf and aarch64_be-none-elf targets

OK for gcc-6-branch ?

Thanks
Yvan

gcc/ChangeLog
2017-06-22  Yvan Roux  <yvan.roux@linaroi.org>

        PR target/79041
        Backport from mainline
        2016-06-20  Wilco Dijkstra  <wdijkstr@arm.com>

        * config/aarch64/aarch64.opt
        (mpc-relative-literal-loads): Rename internal option name.
        * config/aarch64/aarch64.c
        (aarch64_nopcrelative_literal_loads): Rename to
        aarch64_pcrelative_literal_loads.
        (aarch64_expand_mov_immediate): Likewise.
        (aarch64_secondary_reload): Likewise.
        (aarch64_can_use_per_function_literal_pools_p): Likewise.
        (aarch64_classify_symbol): Likewise.
        (aarch64_override_options_after_change_1): Rename and simplify logic.

        2016-01-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

        * config/aarch64/aarch64-protos.h(aarch64_nopcrelative_literal_loads):
        Delete.
        * config/aarch64/aarch64.md
        (aarch64_reload_movcp<GPF_TF:mode><P:mode>): Delete reference to
        aarch64_nopcrelative_literal_loads.
        (aarch64_reload_movcp<VALL:mode><P:mode>): Likewise.

gcc/testsuite/ChangeLog
2017-06-22  Yvan Roux  <yvan.roux@linaroi.org>

        PR target/79041
        * gcc.target/aarch64/pr79041.c: New test.

diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index fa97e29..7b10ff6 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -436,7 +436,6 @@ int aarch64_ccmp_mode_to_code (enum machine_mode mode);
 bool extract_base_offset_in_addr (rtx mem, rtx *base, rtx *offset);
 bool aarch64_operands_ok_for_ldpstp (rtx *, bool, enum machine_mode);
 bool aarch64_operands_adjust_ok_for_ldpstp (rtx *, bool, enum machine_mode);
-extern bool aarch64_nopcrelative_literal_loads;
 
 extern void aarch64_asm_output_pool_epilogue (FILE *, const char *,
 					      tree, HOST_WIDE_INT);
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index e79165b..9b06320 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -152,7 +152,7 @@ enum aarch64_processor aarch64_tune = cortexa53;
 unsigned long aarch64_tune_flags = 0;
 
 /* Global flag for PC relative loads.  */
-bool aarch64_nopcrelative_literal_loads;
+bool aarch64_pcrelative_literal_loads;
 
 /* Support for command line parsing of boolean flags in the tuning
    structures.  */
@@ -1703,7 +1703,7 @@ aarch64_expand_mov_immediate (rtx dest, rtx imm)
 	     we need to expand the literal pool access carefully.
 	     This is something that needs to be done in a number
 	     of places, so could well live as a separate function.  */
-	  if (aarch64_nopcrelative_literal_loads)
+	  if (!aarch64_pcrelative_literal_loads)
 	    {
 	      gcc_assert (can_create_pseudo_p ());
 	      base = gen_reg_rtx (ptr_mode);
@@ -4028,7 +4028,7 @@ aarch64_classify_address (struct aarch64_address_info *info,
 	  return ((GET_CODE (sym) == LABEL_REF
 		   || (GET_CODE (sym) == SYMBOL_REF
 		       && CONSTANT_POOL_ADDRESS_P (sym)
-		       && !aarch64_nopcrelative_literal_loads)));
+		       && aarch64_pcrelative_literal_loads)));
 	}
       return false;
 
@@ -5183,7 +5183,7 @@ aarch64_secondary_reload (bool in_p ATTRIBUTE_UNUSED, rtx x,
   if (MEM_P (x) && GET_CODE (x) == SYMBOL_REF && CONSTANT_POOL_ADDRESS_P (x)
       && (SCALAR_FLOAT_MODE_P (GET_MODE (x))
 	  || targetm.vector_mode_supported_p (GET_MODE (x)))
-      && aarch64_nopcrelative_literal_loads)
+      && !aarch64_pcrelative_literal_loads)
     {
       sri->icode = aarch64_constant_pool_reload_icode (mode);
       return NO_REGS;
@@ -5517,7 +5517,7 @@ aarch64_uxt_size (int shift, HOST_WIDE_INT mask)
 static inline bool
 aarch64_can_use_per_function_literal_pools_p (void)
 {
-  return (!aarch64_nopcrelative_literal_loads
+  return (aarch64_pcrelative_literal_loads
 	  || aarch64_cmodel == AARCH64_CMODEL_LARGE);
 }
 
@@ -8043,32 +8043,31 @@ aarch64_override_options_after_change_1 (struct gcc_options *opts)
 	opts->x_align_functions = aarch64_tune_params.function_align;
     }
 
-  /* If nopcrelative_literal_loads is set on the command line, this
+  /* We default to no pc-relative literal loads.  */
+
+  aarch64_pcrelative_literal_loads = false;
+
+  /* If -mpc-relative-literal-loads is set on the command line, this
      implies that the user asked for PC relative literal loads.  */
-  if (opts->x_nopcrelative_literal_loads == 1)
-    aarch64_nopcrelative_literal_loads = false;
+  if (opts->x_pcrelative_literal_loads == 1)
+    aarch64_pcrelative_literal_loads = true;
 
-  /* If it is not set on the command line, we default to no pc
-     relative literal loads, unless the workaround for Cortex-A53
-     erratum 843419 is in effect.  */
   /* This is PR70113. When building the Linux kernel with
      CONFIG_ARM64_ERRATUM_843419, support for relocations
      R_AARCH64_ADR_PREL_PG_HI21 and R_AARCH64_ADR_PREL_PG_HI21_NC is
      removed from the kernel to avoid loading objects with possibly
-     offending sequences. With nopcrelative_literal_loads, we would
+     offending sequences.  Without -mpc-relative-literal-loads we would
      generate such relocations, preventing the kernel build from
      succeeding.  */
-  if (opts->x_nopcrelative_literal_loads == 2
-      && !TARGET_FIX_ERR_A53_843419)
-    aarch64_nopcrelative_literal_loads = true;
+  if (opts->x_pcrelative_literal_loads == 2
+      && TARGET_FIX_ERR_A53_843419)
+    aarch64_pcrelative_literal_loads = true;
 
-  /* In the tiny memory model it makes no sense
-     to disallow non PC relative literal pool loads
-     as many other things will break anyway.  */
-  if (opts->x_nopcrelative_literal_loads
-      && (aarch64_cmodel == AARCH64_CMODEL_TINY
-	  || aarch64_cmodel == AARCH64_CMODEL_TINY_PIC))
-    aarch64_nopcrelative_literal_loads = false;
+  /* In the tiny memory model it makes no sense to disallow PC relative
+     literal pool loads.  */
+  if (aarch64_cmodel == AARCH64_CMODEL_TINY
+      || aarch64_cmodel == AARCH64_CMODEL_TINY_PIC)
+    aarch64_pcrelative_literal_loads = true;
 }
 
 /* 'Unpack' up the internal tuning structs and update the options
@@ -9314,7 +9313,7 @@ aarch64_classify_symbol (rtx x, rtx offset)
 	  /* This is alright even in PIC code as the constant
 	     pool reference is always PC relative and within
 	     the same translation unit.  */
-	  if (nopcrelative_literal_loads
+	  if (!aarch64_pcrelative_literal_loads
 	      && CONSTANT_POOL_ADDRESS_P (x))
 	    return SYMBOL_SMALL_ABSOLUTE;
 	  else
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 2c9f48c..8c3e216 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -4775,7 +4775,7 @@
  [(set (match_operand:GPF_TF 0 "register_operand" "=w")
        (mem:GPF_TF (match_operand 1 "aarch64_constant_pool_symref" "S")))
   (clobber (match_operand:P 2 "register_operand" "=&r"))]
- "TARGET_FLOAT && aarch64_nopcrelative_literal_loads"
+ "TARGET_FLOAT"
  {
    aarch64_expand_mov_immediate (operands[2], XEXP (operands[1], 0));
    emit_move_insn (operands[0], gen_rtx_MEM (<GPF_TF:MODE>mode, operands[2]));
@@ -4788,7 +4788,7 @@
  [(set (match_operand:VALL 0 "register_operand" "=w")
        (mem:VALL (match_operand 1 "aarch64_constant_pool_symref" "S")))
   (clobber (match_operand:P 2 "register_operand" "=&r"))]
- "TARGET_FLOAT && aarch64_nopcrelative_literal_loads"
+ "TARGET_FLOAT"
  {
    aarch64_expand_mov_immediate (operands[2], XEXP (operands[1], 0));
    emit_move_insn (operands[0], gen_rtx_MEM (<VALL:MODE>mode, operands[2]));
diff --git a/gcc/config/aarch64/aarch64.opt b/gcc/config/aarch64/aarch64.opt
index c637ff4..bc50ec9 100644
--- a/gcc/config/aarch64/aarch64.opt
+++ b/gcc/config/aarch64/aarch64.opt
@@ -146,7 +146,7 @@ EnumValue
 Enum(aarch64_abi) String(lp64) Value(AARCH64_ABI_LP64)
 
 mpc-relative-literal-loads
-Target Report Save Var(nopcrelative_literal_loads) Init(2) Save
+Target Report Save Var(pcrelative_literal_loads) Init(2) Save
 PC relative literal loads.
 
 mlow-precision-recip-sqrt
diff --git a/gcc/testsuite/gcc.target/aarch64/pr79041.c b/gcc/testsuite/gcc.target/aarch64/pr79041.c
new file mode 100644
index 0000000..9ec97b2
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr79041.c
@@ -0,0 +1,26 @@
+/* PR target/79041.  Check that we don't generate the LO12 relocations
+   for -mpc-relative-literal-loads.  */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mcmodel=large -mpc-relative-literal-loads" } */
+
+extern int strcmp(const char *, const char *);
+extern char * strcpy(char *,const char *);
+
+static struct {
+    char *b;
+    char *c;
+} d[] = {
+      { "0", "000000000000000" },
+      { "1", "111111111111111" },
+};
+
+void
+e (const char *b, char *c)
+{
+    int i;
+    for (i = 0; i < 1; ++i)
+      if (!strcmp(d[i].b, b))
+	strcpy(c, d[i].c);
+}
+
+/* { dg-final { scan-assembler-not ":lo12:" } } */