This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[lra] Cleanup the use of offmemok and don't count spilling cost for it


On 29/06/16 22:31, Jiong Wang wrote:
Andreas Krebbel writes:

On 06/28/2016 04:16 PM, Jiong Wang wrote:
...
So my first impression is TARGET_LEGITIMATE_ADDRESS_P on s390 do need a
fix here. The following draft patch fix this, my fix may be in
correct as normally we will allow illegal constant offset if it's
associated with virtual frame register before virtual register
elimination, I don't know if that should be considered here. Also I
don't know if such check should be constrainted under some architecture
features.

--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -4374,6 +4374,11 @@ s390_legitimate_address_p (machine_mode mode, rtx
addr, bool strict)
                 || REGNO_REG_CLASS (REGNO (ad.indx)) == ADDR_REGS))
            return false;
       }
+
+  if (ad.disp
+      && !s390_short_displacement (ad.disp))
+    return false;
+
     return true;
   }
Whether short displacement is required or not depends on the instruction. We always relied on reload
to take care of this.  legitimate_address_p needs to accept the union of all valid adresses on
S/390.  That change probably would cause a severe performance regression.

Meanwhile I feel LRA might be too strict here, as we can't assume all
address legitimized before lra, right? we still need to handle reload
illegal address. While now, a reload of

    set (mem (reg + reg + offset)) regA

    (The offset is out of range that the inner address doesn't pass
     constraint check.)

into

     regB <- reg + reg + offset
    (set (mem (regB)) regA)
This is exactly what is supposed to happen in that case. All addresses which might be invalid to be
used directly in an insn can be handled with load address or load address relative long (+ secondary
reload for odd addresses).

is treate as spill after r237277, so I feel the following check in
lra-constraints.c also needs a relax?

if (no_regs_p && !(REG_P (op) && hard_regno[nop] < 0))


Comments?
I am testing a fix which teach LRA don't add spill cost if it's "offmemok".
Here is the patch,

From my understanding, "offmemok" is used to represent a memory operand
who's address we want to reload, and searching of it's reference location
seems confirmed my understanding as it's always used together with MEM_P check.

So this patch does the following modifications:

  * Only set offmemok to true if MEM_P is also true, as otherwise offmemok
    is not used.
  * Remove redundant MEM_P check which was used together with offmemok.
  * Avoid the addition of spilling cost if offmemok be true as an address
    calculation reload is not spilling.

bootstrap & gcc/g++ regression OK on x86_64/aarch64/arm.

OK for trunk?

2016-06-30  Jiong Wang  <jiong.wang@arm.com>

gcc
  * lra-constraints.c (process_alt_operands): Only set "offmemok" for
  MEM_P.  Remove redundant MEM_P check if it's used together with
  "offmemok" check.  Don't add spilling cost for "offmemok".
  (curr_insn_transform): Remove redundant MEM_P check.

diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index bf08dce..4546f1e 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -1967,7 +1967,6 @@ process_alt_operands (int only_alternative)
 			   combination, because we can't reload
 			   it.	*/
 			if (curr_alt_offmemok[m]
-			    && MEM_P (*curr_id->operand_loc[m])
 			    && curr_alt[m] == NO_REGS && ! curr_alt_win[m])
 			  continue;
 		      }
@@ -2089,7 +2088,8 @@ process_alt_operands (int only_alternative)
 			  || equiv_substition_p[nop])
 			badop = false;
 		      constmemok = true;
-		      offmemok = true;
+		      if (MEM_P (op))
+			offmemok = true;
 		      break;
 
 		    case CT_ADDRESS:
@@ -2436,8 +2436,7 @@ process_alt_operands (int only_alternative)
 		  reject += LRA_MAX_REJECT;
 		}
 
-	      if (! (MEM_P (op) && offmemok)
-		  && ! (const_to_mem && constmemok))
+	      if (!offmemok && ! (const_to_mem && constmemok))
 		{
 		  /* We prefer to reload pseudos over reloading other
 		     things, since such reloads may be able to be
@@ -2488,7 +2487,9 @@ process_alt_operands (int only_alternative)
 
 		 Code below increases the reject for both pseudo and non-pseudo
 		 spill.  */
-	      if (no_regs_p && !(REG_P (op) && hard_regno[nop] < 0))
+	      if (no_regs_p
+		  && !offmemok
+		  && !(REG_P (op) && hard_regno[nop] < 0))
 		{
 		  if (lra_dump_file != NULL)
 		    fprintf
@@ -3909,7 +3910,7 @@ curr_insn_transform (bool check_only_p)
 	 appearing where an offsettable address will do.  It also may
 	 be a case when the address should be special in other words
 	 not a general one (e.g. it needs no index reg).  */
-      if (goal_alt_matched[i][0] == -1 && goal_alt_offmemok[i] && MEM_P (op))
+      if (goal_alt_matched[i][0] == -1 && goal_alt_offmemok[i])
 	{
 	  enum reg_class rclass;
 	  rtx *loc = &XEXP (op, 0);

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]