diff mbox

patch to fix PR69461

Message ID 20160203204921.GA25127@ibm-tiger.the-meissners.org
State New
Headers show

Commit Message

Michael Meissner Feb. 3, 2016, 8:49 p.m. UTC
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.

Comments

Vladimir Makarov Feb. 4, 2016, 3:13 a.m. UTC | #1
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.
Michael Meissner Feb. 4, 2016, 8:07 p.m. UTC | #2
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.
diff mbox

Patch

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