patch to fix PR69461

patch to fix PR69461

[Vladimir Makarov]

[-- Attachment #1: Type: text/plain, Size: 811 bytes --]

   The following patch fixes

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461

   The patch actually solves several issues.  Before the patch LRA has 
 >800 more failures on GCC testsuite on power8.  After the patch the LRA 
has the same number of failures as reload.

Working on the patch, I think I found some typo in 
rs6000.c::rs6000_legitimate_address_p.  The code suspicious to me:

   if (reg_offset_p && reg_addr[mode].fused_toc && 
toc_fusion_mem_wrapped (x, mode))
     return 1;

The function works with address (x) but toc_fusion_mem_wrapped requires 
memory instead of address.  Therefore the function never returns 1 for 
toc_fusion_wrapped address.

Mike and Peter, what do you think about this code?

Anyway, the patch was successfully bootstrapped and tested on power8.

Committed as rev..


[-- Attachment #2: pr69461.patch --]
[-- Type: text/x-patch, Size: 3775 bytes --]

Index: ChangeLog
===================================================================
--- ChangeLog	(revision 233106)
+++ ChangeLog	(working copy)
@@ -1,3 +1,12 @@
+2016-02-03  Vladimir Makarov  <vmakarov@redhat.com>
+	    Alexandre Oliva  <aoliva@redhat.com>
+
+	PR target/69461
+	* lra-constraints.c (simplify_operand_subreg): Check additionally
+	address validity after potential reloading.
+	(process_address_1): Check insns validity.  In case of failure do
+	nothing.
+
 2016-02-03  Kirill Yukhin  <kirill.yukhin@intel.com>
 
 	PR target/69118
Index: testsuite/ChangeLog
===================================================================
--- testsuite/ChangeLog	(revision 233106)
+++ testsuite/ChangeLog	(working copy)
@@ -1,3 +1,9 @@
+2016-02-03  Vladimir Makarov  <vmakarov@redhat.com>
+	    Alexandre Oliva  <aoliva@redhat.com>
+
+	PR target/69461
+	* gcc.target/powerpc/pr69461.c: New.
+
 2016-02-03  Uros Bizjak  <ubizjak@gmail.com>
 
 	* lib/tsan-dg.exp (tsan_init): Move check if tsan executable
Index: lra-constraints.c
===================================================================
--- lra-constraints.c	(revision 233106)
+++ lra-constraints.c	(working copy)
@@ -1411,6 +1411,21 @@ simplify_operand_subreg (int nop, machin
 	  || valid_address_p (GET_MODE (subst), XEXP (subst, 0),
 			      MEM_ADDR_SPACE (subst)))
 	return true;
+      else if ((get_constraint_type (lookup_constraint
+				     (curr_static_id->operand[nop].constraint))
+		!= CT_SPECIAL_MEMORY)
+	       /* We still can reload address and if the address is
+		  valid, we can remove subreg without reloading its
+		  inner memory.  */
+	       && valid_address_p (GET_MODE (subst),
+				   regno_reg_rtx
+				   [ira_class_hard_regs
+				    [base_reg_class (GET_MODE (subst),
+						     MEM_ADDR_SPACE (subst),
+						     ADDRESS, SCRATCH)][0]],
+				   MEM_ADDR_SPACE (subst)))
+	return true;
+
       /* If the address was valid and became invalid, prefer to reload
 	 the memory.  Typical case is when the index scale should
 	 correspond the memory.  */
@@ -2958,6 +2973,8 @@ process_address_1 (int nop, bool check_o
     {
       if (ad.index == NULL)
 	{
+	  rtx_insn *insn;
+	  rtx_insn *last = get_last_insn ();
 	  int code = -1;
 	  enum reg_class cl = base_reg_class (ad.mode, ad.as,
 					      SCRATCH, SCRATCH);
@@ -2966,9 +2983,6 @@ process_address_1 (int nop, bool check_o
 	  new_reg = lra_create_new_reg (Pmode, NULL_RTX, cl, "addr");
 	  if (HAVE_lo_sum)
 	    {
-	      rtx_insn *insn;
-	      rtx_insn *last = get_last_insn ();
-
 	      /* addr => lo_sum (new_base, addr), case (2) above.  */
 	      insn = emit_insn (gen_rtx_SET
 				(new_reg,
@@ -3004,6 +3018,20 @@ process_address_1 (int nop, bool check_o
 	    {
 	      /* addr => new_base, case (2) above.  */
 	      lra_emit_move (new_reg, addr);
+
+	      for (insn = last == NULL_RTX ? get_insns () : NEXT_INSN (last);
+		   insn != NULL_RTX;
+		   insn = NEXT_INSN (insn))
+		if (recog_memoized (insn) < 0)
+		  break;
+	      if (insn != NULL_RTX)
+		{
+		  /* Do nothing if we cannot generate right insns.
+		     This is analogous to reload pass behaviour.  */
+		  delete_insns_since (last);
+		  end_sequence ();
+		  return false;
+		}
 	      *ad.inner = new_reg;
 	    }
 	}
Index: testsuite/gcc.target/powerpc/pr69461.c
===================================================================
--- testsuite/gcc.target/powerpc/pr69461.c	(revision 0)
+++ testsuite/gcc.target/powerpc/pr69461.c	(working copy)
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -mlra" } */
+
+extern void _setjmp (void);
+typedef struct {
+  double real;
+  double imag;
+} Py_complex;
+Py_complex a;
+Py_complex fn1();
+Py_complex fn2() { return fn1(); }
+void fn3() {
+  _setjmp();
+  a = fn2();
+}

Re: patch to fix PR69461

[Michael Meissner]

On Wed, Feb 03, 2016 at 01:02:57PM -0500, Vladimir Makarov wrote:
>   The following patch fixes
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461
> 
>   The patch actually solves several issues.  Before the patch LRA
> has >800 more failures on GCC testsuite on power8.  After the patch
> the LRA has the same number of failures as reload.
> 
> Working on the patch, I think I found some typo in
> rs6000.c::rs6000_legitimate_address_p.  The code suspicious to me:
> 
>   if (reg_offset_p && reg_addr[mode].fused_toc &&
> toc_fusion_mem_wrapped (x, mode))
>     return 1;
> 
> The function works with address (x) but toc_fusion_mem_wrapped
> requires memory instead of address.  Therefore the function never
> returns 1 for toc_fusion_wrapped address.
> 
> Mike and Peter, what do you think about this code?
> 
> Anyway, the patch was successfully bootstrapped and tested on power8.
> 
> Committed as rev..

It looks like it would solve the problem (not knowing the inner details of
lra).

You are correct about the call to toc_fusion_wrapped expecting a MEM, and
rs6000_legitimate_address_p was pass the address.

We are testing the following patch to fix this:

2016-02-03  Michael Meissner  <meissner@linux.vnet.ibm.com>
	    Vladimir Makarov  <vmakarov@redhat.com>

	* config/rs6000/rs6000.c (rs6000_legitimate_address_p): Fix thinko
	in validating fused toc addresses.

Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 233107)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -8399,7 +8399,8 @@ rs6000_legitimate_address_p (machine_mod
       && legitimate_constant_pool_address_p (x, mode,
 					     reg_ok_strict || lra_in_progress))
     return 1;
-  if (reg_offset_p && reg_addr[mode].fused_toc && toc_fusion_mem_wrapped (x, mode))
+  if (reg_offset_p && reg_addr[mode].fused_toc && GET_CODE (x) == UNSPEC
+      && XINT (x, 1) == UNSPEC_FUSION_ADDIS)
     return 1;
   /* For TImode, if we have load/store quad and TImode in VSX registers, only
      allow register indirect addresses.  This will allow the values to go in

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797

Re: patch to fix PR69461

[Vladimir Makarov]

On 02/03/2016 03:49 PM, Michael Meissner wrote:
> On Wed, Feb 03, 2016 at 01:02:57PM -0500, Vladimir Makarov wrote:
>>    The following patch fixes
>>
>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69461
>>
>>    The patch actually solves several issues.  Before the patch LRA
>> has >800 more failures on GCC testsuite on power8.  After the patch
>> the LRA has the same number of failures as reload.
>>
>> Working on the patch, I think I found some typo in
>> rs6000.c::rs6000_legitimate_address_p.  The code suspicious to me:
>>
>>    if (reg_offset_p && reg_addr[mode].fused_toc &&
>> toc_fusion_mem_wrapped (x, mode))
>>      return 1;
>>
>> The function works with address (x) but toc_fusion_mem_wrapped
>> requires memory instead of address.  Therefore the function never
>> returns 1 for toc_fusion_wrapped address.
>>
>> Mike and Peter, what do you think about this code?
>>
>> Anyway, the patch was successfully bootstrapped and tested on power8.
>>
>> Committed as rev..
> It looks like it would solve the problem (not knowing the inner details of
> lra).
>
> You are correct about the call to toc_fusion_wrapped expecting a MEM, and
> rs6000_legitimate_address_p was pass the address.
>
> We are testing the following patch to fix this:
>
> 2016-02-03  Michael Meissner  <meissner@linux.vnet.ibm.com>
> 	    Vladimir Makarov  <vmakarov@redhat.com>
>
> 	* config/rs6000/rs6000.c (rs6000_legitimate_address_p): Fix thinko
> 	in validating fused toc addresses.
>
>
Thanks, Mike.  I found it when trying to fix numerous LRA failures on 
power8.  Reload pass when can not figure out what to do with 
illegitimate address does nothing.  LRA tried still to do something.   
I've made LRA working as reload for this case.  So LRA failures were 
fixed even if fused to wrapper address was believed to be illegitimate.

Still it makes a sense to consider the address legitimate as a lot of 
code in GCC needs this.

Re: patch to fix PR69461

[Michael Meissner]

On Wed, Feb 03, 2016 at 10:13:21PM -0500, Vladimir Makarov wrote:
> Thanks, Mike.  I found it when trying to fix numerous LRA failures
> on power8.  Reload pass when can not figure out what to do with
> illegitimate address does nothing.  LRA tried still to do something.
> I've made LRA working as reload for this case.  So LRA failures were
> fixed even if fused to wrapper address was believed to be
> illegitimate.
> 
> Still it makes a sense to consider the address legitimate as a lot
> of code in GCC needs this.

Yeah, it was simple thinko on my part.  Segher was trying to track down this as
-flto in particular seemed to trigger this.

-- 
Michael Meissner, IBM
IBM, M/S 2506R, 550 King Street, Littleton, MA 01460-6245, USA
email: meissner@linux.vnet.ibm.com, phone: +1 (978) 899-4797