Patchwork [AArch64] Handle symbol + offset more effectively

login
register
mail settings
Submitter Ian Bolton
Date Sept. 25, 2012, 1:45 p.m.
Message ID <000001cd9b24$173eec20$45bcc460$@bolton@arm.com>
Download mbox | patch
Permalink /patch/186819/
State New
Headers show

Comments

Ian Bolton - Sept. 25, 2012, 1:45 p.m.
Hi all,

This patch corrects what seemed to be a typo in expand_mov_immediate
in aarch64.c, where we had || instead of an && in our original code.

if (offset != const0_rtx
      && (targetm.cannot_force_const_mem (mode, imm)
	  || (can_create_pseudo_p ())))  // <----- should have been &&

At any given time, this code would have treated all input the same
and will have caused all non-zero offsets to have been forced to
temporaries, and made us never run the code in the remainder of the
function.

In terms of measurable impact, this patch provides a better fix to the
problem I was trying to solve with this patch:

http://gcc.gnu.org/ml/gcc-patches/2012-08/msg02072.html

Almost all credit should go to Richard Henderson for this patch.
It is all his, but for a minor change I made to some predicates which
now become relevant when we execute more of the expand_mov_immediate
function.

My testing showed no regressions for bare-metal or linux.

OK for aarch64-branch and aarch64-4.7-branch?

Cheers,
Ian


2012-09-25  Richard Henderson  <rth@redhat.com>
            Ian Bolton  <ian.bolton@arm.com>

	* config/aarch64/aarch64.c (aarch64_expand_mov_immediate): Fix a
	functional typo and refactor code in switch statement.
	* config/aarch64/aarch64.md (add_losym): Handle symbol + offset.
	* config/aarch64/predicates.md (aarch64_tls_ie_symref): Match const.
	(aarch64_tls_le_symref): Likewise.
Marcus Shawcroft - Sept. 27, 2012, 5:06 p.m.
On 25/09/12 14:45, Ian Bolton wrote:
> Hi all,
>
> This patch corrects what seemed to be a typo in expand_mov_immediate
> in aarch64.c, where we had || instead of an&&  in our original code.
>
> if (offset != const0_rtx
>        &&  (targetm.cannot_force_const_mem (mode, imm)
> 	  || (can_create_pseudo_p ())))  //<----- should have been&&
>
> At any given time, this code would have treated all input the same
> and will have caused all non-zero offsets to have been forced to
> temporaries, and made us never run the code in the remainder of the
> function.
>
> In terms of measurable impact, this patch provides a better fix to the
> problem I was trying to solve with this patch:
>
> http://gcc.gnu.org/ml/gcc-patches/2012-08/msg02072.html
>
> Almost all credit should go to Richard Henderson for this patch.
> It is all his, but for a minor change I made to some predicates which
> now become relevant when we execute more of the expand_mov_immediate
> function.
>
> My testing showed no regressions for bare-metal or linux.
>
> OK for aarch64-branch and aarch64-4.7-branch?
>
> Cheers,
> Ian
>
>
> 2012-09-25  Richard Henderson<rth@redhat.com>
>              Ian Bolton<ian.bolton@arm.com>
>
> 	* config/aarch64/aarch64.c (aarch64_expand_mov_immediate): Fix a
> 	functional typo and refactor code in switch statement.
> 	* config/aarch64/aarch64.md (add_losym): Handle symbol + offset.
> 	* config/aarch64/predicates.md (aarch64_tls_ie_symref): Match const.
> 	(aarch64_tls_le_symref): Likewise.

OK for aarch64-branch and backport to aarch64-4.7-branch.
/Marcus

Patch

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 2d7eba7..edeee30 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -652,43 +652,57 @@  aarch64_expand_mov_immediate (rtx dest, rtx imm)
   unsigned HOST_WIDE_INT val;
   bool subtargets;
   rtx subtarget;
-  rtx base, offset;
   int one_match, zero_match;
 
   gcc_assert (mode == SImode || mode == DImode);
 
-  /* If we have (const (plus symbol offset)), and that expression cannot
-     be forced into memory, load the symbol first and add in the offset.  */
-  split_const (imm, &base, &offset);
-  if (offset != const0_rtx
-      && (targetm.cannot_force_const_mem (mode, imm)
-	  || (can_create_pseudo_p ())))
-    {
-      base = aarch64_force_temporary (dest, base);
-      aarch64_emit_move (dest, aarch64_add_offset (mode, NULL, base, INTVAL (offset)));
-      return;
-    }
-
   /* Check on what type of symbol it is.  */
-  if (GET_CODE (base) == SYMBOL_REF || GET_CODE (base) == LABEL_REF)
+  if (GET_CODE (imm) == SYMBOL_REF
+      || GET_CODE (imm) == LABEL_REF
+      || GET_CODE (imm) == CONST)
     {
-      rtx mem;
-      switch (aarch64_classify_symbol (base, SYMBOL_CONTEXT_ADR))
+      rtx mem, base, offset;
+      enum aarch64_symbol_type sty;
+
+      /* If we have (const (plus symbol offset)), separate out the offset
+	 before we start classifying the symbol.  */
+      split_const (imm, &base, &offset);
+
+      sty = aarch64_classify_symbol (base, SYMBOL_CONTEXT_ADR);
+      switch (sty)
 	{
 	case SYMBOL_FORCE_TO_MEM:
-	  mem  = force_const_mem (mode, imm);
+	  if (offset != const0_rtx
+	      && targetm.cannot_force_const_mem (mode, imm))
+	    {
+	      gcc_assert(can_create_pseudo_p ());
+	      base = aarch64_force_temporary (dest, base);
+	      base = aarch64_add_offset (mode, NULL, base, INTVAL (offset));
+	      aarch64_emit_move (dest, base);
+	      return;
+	    }
+	  mem = force_const_mem (mode, imm);
 	  gcc_assert (mem);
 	  emit_insn (gen_rtx_SET (VOIDmode, dest, mem));
 	  return;
 
-    case SYMBOL_SMALL_TLSGD:
-    case SYMBOL_SMALL_TLSDESC:
-    case SYMBOL_SMALL_GOTTPREL:
-    case SYMBOL_SMALL_TPREL:
+        case SYMBOL_SMALL_TLSGD:
+        case SYMBOL_SMALL_TLSDESC:
+        case SYMBOL_SMALL_GOTTPREL:
 	case SYMBOL_SMALL_GOT:
+	  if (offset != const0_rtx)
+	    {
+	      gcc_assert(can_create_pseudo_p ());
+	      base = aarch64_force_temporary (dest, base);
+	      base = aarch64_add_offset (mode, NULL, base, INTVAL (offset));
+	      aarch64_emit_move (dest, base);
+	      return;
+	    }
+	  /* FALLTHRU */
+
+        case SYMBOL_SMALL_TPREL:
 	case SYMBOL_SMALL_ABSOLUTE:
-	  aarch64_load_symref_appropriately
-	    (dest, imm, aarch64_classify_symbol (base, SYMBOL_CONTEXT_ADR));
+	  aarch64_load_symref_appropriately (dest, imm, sty);
 	  return;
 
 	default:
@@ -696,7 +710,7 @@  aarch64_expand_mov_immediate (rtx dest, rtx imm)
 	}
     }
 
-  if ((CONST_INT_P (imm) && aarch64_move_imm (INTVAL (imm), mode)))
+  if (CONST_INT_P (imm) && aarch64_move_imm (INTVAL (imm), mode))
     {
       emit_insn (gen_rtx_SET (VOIDmode, dest, imm));
       return;
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index b399ab4..3834558 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -2840,7 +2840,7 @@ 
 	(lo_sum:DI (match_operand:DI 1 "register_operand" "r")
 		   (match_operand 2 "aarch64_valid_symref" "S")))]
   ""
-  "add\\t%0, %1, :lo12:%2"
+  "add\\t%0, %1, :lo12:%a2"
   [(set_attr "v8type" "alu")
    (set_attr "mode" "DI")]
 
diff --git a/gcc/config/aarch64/predicates.md b/gcc/config/aarch64/predicates.md
index 328e5cf..fb4d84f 100644
--- a/gcc/config/aarch64/predicates.md
+++ b/gcc/config/aarch64/predicates.md
@@ -122,7 +122,7 @@ 
 })
 
 (define_predicate "aarch64_tls_ie_symref"
-  (match_code "symbol_ref, label_ref")
+  (match_code "const, symbol_ref, label_ref")
 {
   switch (GET_CODE (op))
     {
@@ -143,7 +143,7 @@ 
 })
 
 (define_predicate "aarch64_tls_le_symref"
-  (match_code "symbol_ref, label_ref")
+  (match_code "const, symbol_ref, label_ref")
 {
   switch (GET_CODE (op))
     {