Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

The test case gcc.c-torture/compile/20010102-1.c is failing on IA64
HP-UX in 32 bit mode when compiled with -O3 -funroll-loops.  My analysis
shows that this is because regrename is not copying the REG_POINTER
attribute of a register when renaming it.  This causes a problem with
the use of the IA64 addp4 instruction which, when used with two
registers, requires that one of the registers be a pointer and the other
be an offset (and that we know which one is which).  Without this
patch neither register is marked as a pointer.

This patch copies the REG_POINTER setting of a register whenever we
are copying the attributes.  Tested on IA64 HP-UX and Linux with
no regressions.

I am cc'ing Paolo and Peter Bergner because this problem started with
the patch to PR 28690.

OK for checkin?

Steve Ellcey
sje@cup.hp.com


2008-08-16  Steve Ellcey  <sje@cup.hp.com>

	* regrename.c (do_replace): Copy REG_POINTER value to new reg.
	(find_oldest_value_reg): Ditto.
	(copyprop_hardreg_forward_1): Ditto.


Index: regrename.c
===================================================================
--- regrename.c	(revision 140386)
+++ regrename.c	(working copy)
@@ -357,11 +357,13 @@ do_replace (struct du_chain *chain, int 
     {
       unsigned int regno = ORIGINAL_REGNO (*chain->loc);
       struct reg_attrs * attr = REG_ATTRS (*chain->loc);
+      int reg_ptr = REG_POINTER (*chain->loc);
 
       *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
       if (regno >= FIRST_PSEUDO_REGISTER)
 	ORIGINAL_REGNO (*chain->loc) = regno;
       REG_ATTRS (*chain->loc) = attr;
+      REG_POINTER (*chain->loc) = reg_ptr;
       df_insn_rescan (chain->insn);
       chain = chain->next_use;
     }
@@ -1375,6 +1377,7 @@ find_oldest_value_reg (enum reg_class cl
 	{
 	  ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
 	  REG_ATTRS (new_rtx) = REG_ATTRS (reg);
+	  REG_POINTER (new_rtx) = REG_POINTER (reg);
 	  return new_rtx;
 	}
     }
@@ -1673,6 +1676,7 @@ copyprop_hardreg_forward_1 (basic_block 
 		    {
 		      ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
 		      REG_ATTRS (new_rtx) = REG_ATTRS (src);
+		      REG_POINTER (new_rtx) = REG_POINTER (src);
 		      if (dump_file)
 			fprintf (dump_file,
 				 "insn %u: replaced reg %u with %u\n",

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Peter Bergner]

On Tue, 2008-09-16 at 09:51 -0700, Steve Ellcey wrote:
> Index: regrename.c
> ===================================================================
> --- regrename.c	(revision 140386)
> +++ regrename.c	(working copy)
> @@ -357,11 +357,13 @@ do_replace (struct du_chain *chain, int 
>      {
>        unsigned int regno = ORIGINAL_REGNO (*chain->loc);
>        struct reg_attrs * attr = REG_ATTRS (*chain->loc);
> +      int reg_ptr = REG_POINTER (*chain->loc);
> 
>        *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
>        if (regno >= FIRST_PSEUDO_REGISTER)
>  	ORIGINAL_REGNO (*chain->loc) = regno;
>        REG_ATTRS (*chain->loc) = attr;
> +      REG_POINTER (*chain->loc) = reg_ptr;
>        df_insn_rescan (chain->insn);
>        chain = chain->next_use;
>      }
> @@ -1375,6 +1377,7 @@ find_oldest_value_reg (enum reg_class cl
>  	{
>  	  ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
>  	  REG_ATTRS (new_rtx) = REG_ATTRS (reg);
> +	  REG_POINTER (new_rtx) = REG_POINTER (reg);
>  	  return new_rtx;
>  	}
>      }
> @@ -1673,6 +1676,7 @@ copyprop_hardreg_forward_1 (basic_block 
>  		    {
>  		      ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
>  		      REG_ATTRS (new_rtx) = REG_ATTRS (src);
> +		      REG_POINTER (new_rtx) = REG_POINTER (src);
>  		      if (dump_file)
>  			fprintf (dump_file,
>  				 "insn %u: replaced reg %u with %u\n",

Would it make more sense to call gen_reg_rtx_and_attrs() and
set_reg_attrs_from_value() to get/set the REG_ATTRS and REG_POINTER
fields?  There was also a bug (PR36533), where we shouldn't set the
REG_POINTER attribute for hard registers.  I'm not sure if that's
possible here, but it doesn't seem to be checked.  Using
set_reg_attrs_from_value() would protect us from that.

Peter

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Tue, 2008-09-16 at 13:20 -0500, Peter Bergner wrote:

> Would it make more sense to call gen_reg_rtx_and_attrs() and
> set_reg_attrs_from_value() to get/set the REG_ATTRS and REG_POINTER
> fields?  There was also a bug (PR36533), where we shouldn't set the
> REG_POINTER attribute for hard registers.  I'm not sure if that's
> possible here, but it doesn't seem to be checked.  Using
> set_reg_attrs_from_value() would protect us from that.
> 
> Peter

I think I need to have REG_POINTER set on hard registers.  The failure I
was getting is:

(insn:TI 1149 1480 1148 45 (set (reg:DI 2 r2 [699])
        (unspec:DI [
                (plus:SI (reg:SI 3 r3 [orig:697 <variable>.object_base ]
[697])
                    (reg/v:SI 15 r15 [orig:627 i.69 ] [627]))
            ] 24)) -1 (nil))
20010102-1.c:101: internal compiler error: in get_attr_first_insn, at
config/ia64/itanium2.md:1838
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

The problem is that neither r3 or r15 are marked as pointers and so I
don't match my addp4 instruction template.  Does the REG_POINTER
attribute have to be true or false for the entire life of the hard
register?  It seems obvious that as different values are put into a hard
register, sometimes it will have a pointer and sometimes it will not.

I guess I am not sure why setting REG_POINTER on a hard register is bad.
I looked at PR 36533 but it did not enlighten me.

Steve Ellcey
sje@cup.hp.com

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Peter Bergner]

On Tue, 2008-09-16 at 11:47 -0700, Steve Ellcey wrote:
> On Tue, 2008-09-16 at 13:20 -0500, Peter Bergner wrote:
> 
> > Would it make more sense to call gen_reg_rtx_and_attrs() and
> > set_reg_attrs_from_value() to get/set the REG_ATTRS and REG_POINTER
> > fields?  There was also a bug (PR36533), where we shouldn't set the
> > REG_POINTER attribute for hard registers.  I'm not sure if that's
> > possible here, but it doesn't seem to be checked.  Using
> > set_reg_attrs_from_value() would protect us from that.
> > 
> > Peter
> 
> I think I need to have REG_POINTER set on hard registers.  The failure I
> was getting is:
> 
> (insn:TI 1149 1480 1148 45 (set (reg:DI 2 r2 [699])
>         (unspec:DI [
>                 (plus:SI (reg:SI 3 r3 [orig:697 <variable>.object_base ]
> [697])
>                     (reg/v:SI 15 r15 [orig:627 i.69 ] [627]))
>             ] 24)) -1 (nil))
> 20010102-1.c:101: internal compiler error: in get_attr_first_insn, at
> config/ia64/itanium2.md:1838
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
> 
> The problem is that neither r3 or r15 are marked as pointers and so I
> don't match my addp4 instruction template.  Does the REG_POINTER
> attribute have to be true or false for the entire life of the hard
> register?  It seems obvious that as different values are put into a hard
> register, sometimes it will have a pointer and sometimes it will not.

Correct, the REG_POINTER attribute is true/false for the entire use of
the hard register, even though it can contain many unrelated values and
that was the basic problem.


> I guess I am not sure why setting REG_POINTER on a hard register is bad.
> I looked at PR 36533 but it did not enlighten me.

Jakub, what was the exact problem having the REG_POINTER attribute set on
a hard register cause on x86 again?

Peter

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jakub Jelinek]

On Tue, Sep 16, 2008 at 02:02:37PM -0500, Peter Bergner wrote:
> On Tue, 2008-09-16 at 11:47 -0700, Steve Ellcey wrote:
> > On Tue, 2008-09-16 at 13:20 -0500, Peter Bergner wrote:
> > I guess I am not sure why setting REG_POINTER on a hard register is bad.
> > I looked at PR 36533 but it did not enlighten me.
> 
> Jakub, what was the exact problem having the REG_POINTER attribute set on
> a hard register cause on x86 again?

The problem is that unlike pseudo registers, a hard register can contain
different kinds of variables over the lifetime of a function.
So, it can be a pointer for part of a function and non-pointer for another
part of the function.  Or it can contain two different pointers with
different alignments, yet REGNO_POINTER_ALIGN is a shared value for
particular REGNO inside of the whole function.

	Jakub

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Peter Bergner]

On Tue, 2008-09-16 at 17:35 -0400, Jakub Jelinek wrote:
> On Tue, Sep 16, 2008 at 02:02:37PM -0500, Peter Bergner wrote:
> > Jakub, what was the exact problem having the REG_POINTER attribute set on
> > a hard register cause on x86 again?
> 
> The problem is that unlike pseudo registers, a hard register can contain
> different kinds of variables over the lifetime of a function.
> So, it can be a pointer for part of a function and non-pointer for another
> part of the function.  Or it can contain two different pointers with
> different alignments, yet REGNO_POINTER_ALIGN is a shared value for
> particular REGNO inside of the whole function.

Right, what I was wondering was what optimization/code generation was
x86 doing that it shouldn't have because the REG_POINTER was erroneously
set on the hard reg?

Peter

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Steve Ellcey wrote:
> On Tue, 2008-09-16 at 13:20 -0500, Peter Bergner wrote:
>
>   
>> Would it make more sense to call gen_reg_rtx_and_attrs() and
>> set_reg_attrs_from_value() to get/set the REG_ATTRS and REG_POINTER
>> fields?  There was also a bug (PR36533), where we shouldn't set the
>> REG_POINTER attribute for hard registers.  I'm not sure if that's
>> possible here, but it doesn't seem to be checked.  Using
>> set_reg_attrs_from_value() would protect us from that.
>>
>> Peter
>>     
>
> I think I need to have REG_POINTER set on hard registers.  The failure I
> was getting is:
>
> (insn:TI 1149 1480 1148 45 (set (reg:DI 2 r2 [699])
>         (unspec:DI [
>                 (plus:SI (reg:SI 3 r3 [orig:697 <variable>.object_base ]
> [697])
>                     (reg/v:SI 15 r15 [orig:627 i.69 ] [627]))
>             ] 24)) -1 (nil))
> 20010102-1.c:101: internal compiler error: in get_attr_first_insn, at
> config/ia64/itanium2.md:1838
> Please submit a full bug report,
> with preprocessed source if appropriate.
> See <http://gcc.gnu.org/bugs.html> for instructions.
>
> The problem is that neither r3 or r15 are marked as pointers and so I
> don't match my addp4 instruction template.  Does the REG_POINTER
> attribute have to be true or false for the entire life of the hard
> register?  It seems obvious that as different values are put into a hard
> register, sometimes it will have a pointer and sometimes it will not.
>
> I guess I am not sure why setting REG_POINTER on a hard register is bad.
> I looked at PR 36533 but it did not enlighten me.
>   

You don't necessarily need to set REG_POINTER on hard registers, and in 
general it's unwise.

What we do  is we set REG_POINTER on appropriate pseudos within the RTL 
for the pseudo.  Then after register allocation/reloading, alter_reg 
changes the REGNO field for the pseudo to turn it into a hard register.  
Note this preserves the REG_POINTER flag so that you can test it after 
reload. 

So you'll need to look into where that insn was created and ensure that 
the appropriate pseudo has REG_POINTER set (r699)  Then verify that it 
was carried forward through allocation/reloading.

REG_POINTER, when set, indicates the object is a pointer for its entire 
lifetime -- thus setting it on hard registers is generally a bad idea 
and it's only safe under certain circumstances on pseudos, particularly 
in the presence of cross jumping.

jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Peter Bergner]

On Tue, 2008-09-16 at 13:14 -0600, Jeff Law wrote:
> You don't necessarily need to set REG_POINTER on hard registers, and in 
> general it's unwise.
> 
> What we do  is we set REG_POINTER on appropriate pseudos within the RTL 
> for the pseudo.  Then after register allocation/reloading, alter_reg 
> changes the REGNO field for the pseudo to turn it into a hard register.  
> Note this preserves the REG_POINTER flag so that you can test it after 
> reload. 
> 
> So you'll need to look into where that insn was created and ensure that 
> the appropriate pseudo has REG_POINTER set (r699)  Then verify that it 
> was carried forward through allocation/reloading.
> 
> REG_POINTER, when set, indicates the object is a pointer for its entire 
> lifetime -- thus setting it on hard registers is generally a bad idea 
> and it's only safe under certain circumstances on pseudos, particularly 
> in the presence of cross jumping.

So something similar to this untested patch to fix Steve's problem?
(This assumes ORIGINAL_REG (reg) == reg if reg is a psuedo.  Maybe it
isn't. :)

Peter

--- config/ia64/predicates.md	(revision 140251)
+++ config/ia64/predicates.md	(working copy)
@@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
 (define_predicate "basereg_operand"
   (match_operand 0 "register_operand")
 {
-  return REG_P (op) && REG_POINTER (op);
+  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO
(op)]);
 })

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Peter Bergner wrote:
> On Tue, 2008-09-16 at 13:14 -0600, Jeff Law wrote:
>   
>> You don't necessarily need to set REG_POINTER on hard registers, and in 
>> general it's unwise.
>>
>> What we do  is we set REG_POINTER on appropriate pseudos within the RTL 
>> for the pseudo.  Then after register allocation/reloading, alter_reg 
>> changes the REGNO field for the pseudo to turn it into a hard register.  
>> Note this preserves the REG_POINTER flag so that you can test it after 
>> reload. 
>>
>> So you'll need to look into where that insn was created and ensure that 
>> the appropriate pseudo has REG_POINTER set (r699)  Then verify that it 
>> was carried forward through allocation/reloading.
>>
>> REG_POINTER, when set, indicates the object is a pointer for its entire 
>> lifetime -- thus setting it on hard registers is generally a bad idea 
>> and it's only safe under certain circumstances on pseudos, particularly 
>> in the presence of cross jumping.
>>     
>
> So something similar to this untested patch to fix Steve's problem?
> (This assumes ORIGINAL_REG (reg) == reg if reg is a psuedo.  Maybe it
> isn't. :)
>
> Peter
>
> --- config/ia64/predicates.md	(revision 140251)
> +++ config/ia64/predicates.md	(working copy)
> @@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
>  (define_predicate "basereg_operand"
>    (match_operand 0 "register_operand")
>  {
> -  return REG_P (op) && REG_POINTER (op);
> +  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO
> (op)]);
>  })
>
>   
But this shouldn't be necessary.  REG_POINTER is set within the RTX for 
the original pseudo and alter_reg (via changing REGNO) preserves the 
setting for use post-reload, even though we're looking at a hard register.

My recommendation for Steve is the same.  First ensure that REG_POINTER 
is set on the appropriate REG within the insn when the insn is created, 
then go forward and pinpoint where it disappears as the disappearance is 
a bug.
Jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Tue, 2008-09-16 at 14:15 -0600, Jeff Law wrote:

> But this shouldn't be necessary.  REG_POINTER is set within the RTX for 
> the original pseudo and alter_reg (via changing REGNO) preserves the 
> setting for use post-reload, even though we're looking at a hard register.
> 
> My recommendation for Steve is the same.  First ensure that REG_POINTER 
> is set on the appropriate REG within the insn when the insn is created, 
> then go forward and pinpoint where it disappears as the disappearance is 
> a bug.
> Jeff

Well, it disappears during the rtx register renaming phase and
I thought I was fixing it by preserving it during register renaming.
This is where it disappears.   If I add -fno-rename-registers the
problem goes away.

Steve Ellcey
sje@cup.hp.com

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Steve Ellcey wrote:
> On Tue, 2008-09-16 at 14:15 -0600, Jeff Law wrote:
>
>   
>> But this shouldn't be necessary.  REG_POINTER is set within the RTX for 
>> the original pseudo and alter_reg (via changing REGNO) preserves the 
>> setting for use post-reload, even though we're looking at a hard register.
>>
>> My recommendation for Steve is the same.  First ensure that REG_POINTER 
>> is set on the appropriate REG within the insn when the insn is created, 
>> then go forward and pinpoint where it disappears as the disappearance is 
>> a bug.
>> Jeff
>>     
>
> Well, it disappears during the rtx register renaming phase and
> I thought I was fixing it by preserving it during register renaming.
> This is where it disappears.   If I add -fno-rename-registers the
> problem goes away.
>
>   
I haven't followed the whole thread -- did someone object to fixing this 
in regrename?  ISTM that if that's what you fixed that you were dead-on 
correct.

Jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Tue, 2008-09-16 at 14:43 -0600, Jeff Law wrote:

> I haven't followed the whole thread -- did someone object to fixing this 
> in regrename?  ISTM that if that's what you fixed that you were dead-on 
> correct.
> 
> Jeff

Peter was questioning whether or not we should be setting REG_POINTER on
hard registers in regrename.c.  But I just did a quick experiment and if
I change my patch to only set REG_POINTER on psuedo-regs then it doesn't
fix the bug.  I did try his predicate change as an alternative:

--- config/ia64/predicates.md   (revision 140251)
+++ config/ia64/predicates.md   (working copy)
@@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
 (define_predicate "basereg_operand"
   (match_operand 0 "register_operand")
 {
-  return REG_P (op) && REG_POINTER (op);
+  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO(op)]);
 })

And that did fix the test case I have (I didn't do a full testing).
But it seems that if we aren't setting REG_POINTER for hard regs in
regrename.c then we shouldn't be copying attributes for them either and
we are doing that.

Steve Ellcey
sje@cup.hp.com

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Peter Bergner]

On Tue, 2008-09-16 at 14:03 -0700, Steve Ellcey wrote:
> Peter was questioning whether or not we should be setting REG_POINTER on
> hard registers in regrename.c.  But I just did a quick experiment and if
> I change my patch to only set REG_POINTER on psuedo-regs then it doesn't
> fix the bug.  I did try his predicate change as an alternative:
> 
> --- config/ia64/predicates.md   (revision 140251)
> +++ config/ia64/predicates.md   (working copy)
> @@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
>  (define_predicate "basereg_operand"
>    (match_operand 0 "register_operand")
>  {
> -  return REG_P (op) && REG_POINTER (op);
> +  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO(op)]);
>  })
> 
> And that did fix the test case I have (I didn't do a full testing).
> But it seems that if we aren't setting REG_POINTER for hard regs in
> regrename.c then we shouldn't be copying attributes for them either and
> we are doing that.

I think Jeff and I agree that setting REG_POINTER on hard regs is wrong,
given the issues discovered in PR36533.  That is why we disabled setting
REG_POINTER and REG_ATTRS in set_reg_attrs_from_value().  I think that
is what is tickling your basereg_operand predicate test, because now
hard regs we used to erroneously set the REG_POINTER, no longer have it
set.  ISTM, the real fix is to change your predicate to check whether
ORIGINAL_REGNO has the REG_POINTER rather than op (like above), since op
can be a hard reg and hard regs shouldn't have REG_POINTER set.
As for the regrename.c change, can't we just do the (untested!) code below
which will not set REG_POINTER or REG_ATTRS on hard regs?

Peter



Index: regrename.c
===================================================================
--- regrename.c	(revision 140251)
+++ regrename.c	(working copy)
@@ -356,12 +356,11 @@ do_replace (struct du_chain *chain, int 
   while (chain)
     {
       unsigned int regno = ORIGINAL_REGNO (*chain->loc);
-      struct reg_attrs * attr = REG_ATTRS (*chain->loc);
 
-      *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
+      *chain->loc = gen_reg_rtx_and_attrs (*chain->loc);
+      SET_REGNO (*chain->loc, reg);
       if (regno >= FIRST_PSEUDO_REGISTER)
 	ORIGINAL_REGNO (*chain->loc) = regno;
-      REG_ATTRS (*chain->loc) = attr;
       df_insn_rescan (chain->insn);
       chain = chain->next_use;
     }
@@ -1374,7 +1373,7 @@ find_oldest_value_reg (enum reg_class cl
       if (new_rtx)
 	{
 	  ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
-	  REG_ATTRS (new_rtx) = REG_ATTRS (reg);
+	  set_reg_attrs_from_value (new_rtx, reg);
 	  return new_rtx;
 	}
     }
@@ -1672,7 +1671,7 @@ copyprop_hardreg_forward_1 (basic_block 
 		  if (validate_change (insn, &SET_SRC (set), new_rtx, 0))
 		    {
 		      ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
-		      REG_ATTRS (new_rtx) = REG_ATTRS (src);
+		      set_reg_attrs_from_value (new_rtx, src);
 		      if (dump_file)
 			fprintf (dump_file,
 				 "insn %u: replaced reg %u with %u\n",
Index: config/ia64/predicates.md
===================================================================
--- config/ia64/predicates.md	(revision 140251)
+++ config/ia64/predicates.md	(working copy)
@@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
 (define_predicate "basereg_operand"
   (match_operand 0 "register_operand")
 {
-  return REG_P (op) && REG_POINTER (op);
+  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
 })
 

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

I put the ia64/predicates.md change through a full bootstrap and test
and it had no regressions and did fix the bug in question.  I'll add the
regrename.c changes to see if that has any impact, but presumably it
won't.

Steve Ellcey
sje@cup.hp.com


On Tue, 2008-09-16 at 19:42 -0500, Peter Bergner wrote:

> Index: regrename.c
> ===================================================================
> --- regrename.c	(revision 140251)
> +++ regrename.c	(working copy)
> @@ -356,12 +356,11 @@ do_replace (struct du_chain *chain, int 
>    while (chain)
>      {
>        unsigned int regno = ORIGINAL_REGNO (*chain->loc);
> -      struct reg_attrs * attr = REG_ATTRS (*chain->loc);
>  
> -      *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
> +      *chain->loc = gen_reg_rtx_and_attrs (*chain->loc);
> +      SET_REGNO (*chain->loc, reg);
>        if (regno >= FIRST_PSEUDO_REGISTER)
>  	ORIGINAL_REGNO (*chain->loc) = regno;
> -      REG_ATTRS (*chain->loc) = attr;
>        df_insn_rescan (chain->insn);
>        chain = chain->next_use;
>      }
> @@ -1374,7 +1373,7 @@ find_oldest_value_reg (enum reg_class cl
>        if (new_rtx)
>  	{
>  	  ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
> -	  REG_ATTRS (new_rtx) = REG_ATTRS (reg);
> +	  set_reg_attrs_from_value (new_rtx, reg);
>  	  return new_rtx;
>  	}
>      }
> @@ -1672,7 +1671,7 @@ copyprop_hardreg_forward_1 (basic_block 
>  		  if (validate_change (insn, &SET_SRC (set), new_rtx, 0))
>  		    {
>  		      ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
> -		      REG_ATTRS (new_rtx) = REG_ATTRS (src);
> +		      set_reg_attrs_from_value (new_rtx, src);
>  		      if (dump_file)
>  			fprintf (dump_file,
>  				 "insn %u: replaced reg %u with %u\n",
> Index: config/ia64/predicates.md
> ===================================================================
> --- config/ia64/predicates.md	(revision 140251)
> +++ config/ia64/predicates.md	(working copy)
> @@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
>  (define_predicate "basereg_operand"
>    (match_operand 0 "register_operand")
>  {
> -  return REG_P (op) && REG_POINTER (op);
> +  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
>  })
>  
> 
> 
> 

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Tue, 2008-09-16 at 19:42 -0500, Peter Bergner wrote:

> I think Jeff and I agree that setting REG_POINTER on hard regs is wrong,
> given the issues discovered in PR36533.  That is why we disabled setting
> REG_POINTER and REG_ATTRS in set_reg_attrs_from_value().  I think that
> is what is tickling your basereg_operand predicate test, because now
> hard regs we used to erroneously set the REG_POINTER, no longer have it
> set.  ISTM, the real fix is to change your predicate to check whether
> ORIGINAL_REGNO has the REG_POINTER rather than op (like above), since op
> can be a hard reg and hard regs shouldn't have REG_POINTER set.
> As for the regrename.c change, can't we just do the (untested!) code below
> which will not set REG_POINTER or REG_ATTRS on hard regs?
> 
> Peter

I tried this patch, it broke on all platforms including x86_64 Linux.
The x86 build worked but many tests failed with:


/proj/opensrc/nightly/src/trunk/gcc/testsuite/gcc.c-torture/compile/20000329-1.c: In function 'giop_encode_ulong':
/proj/opensrc/nightly/src/trunk/gcc/testsuite/gcc.c-torture/compile/20000329-1.c:18: internal compiler error: in gen_reg_rtx, at emit-rtl.c:864

I am going to retest with just the ia64/predicates change.  I thought I
had tested that before but I actually still had a change in regrename.c
during that testing.

Steve Ellcey
sje@cup.hp.com


> Index: regrename.c
> ===================================================================
> --- regrename.c	(revision 140251)
> +++ regrename.c	(working copy)
> @@ -356,12 +356,11 @@ do_replace (struct du_chain *chain, int 
>    while (chain)
>      {
>        unsigned int regno = ORIGINAL_REGNO (*chain->loc);
> -      struct reg_attrs * attr = REG_ATTRS (*chain->loc);
>  
> -      *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
> +      *chain->loc = gen_reg_rtx_and_attrs (*chain->loc);
> +      SET_REGNO (*chain->loc, reg);
>        if (regno >= FIRST_PSEUDO_REGISTER)
>  	ORIGINAL_REGNO (*chain->loc) = regno;
> -      REG_ATTRS (*chain->loc) = attr;
>        df_insn_rescan (chain->insn);
>        chain = chain->next_use;
>      }
> @@ -1374,7 +1373,7 @@ find_oldest_value_reg (enum reg_class cl
>        if (new_rtx)
>  	{
>  	  ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
> -	  REG_ATTRS (new_rtx) = REG_ATTRS (reg);
> +	  set_reg_attrs_from_value (new_rtx, reg);
>  	  return new_rtx;
>  	}
>      }
> @@ -1672,7 +1671,7 @@ copyprop_hardreg_forward_1 (basic_block 
>  		  if (validate_change (insn, &SET_SRC (set), new_rtx, 0))
>  		    {
>  		      ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
> -		      REG_ATTRS (new_rtx) = REG_ATTRS (src);
> +		      set_reg_attrs_from_value (new_rtx, src);
>  		      if (dump_file)
>  			fprintf (dump_file,
>  				 "insn %u: replaced reg %u with %u\n",
> Index: config/ia64/predicates.md
> ===================================================================
> --- config/ia64/predicates.md	(revision 140251)
> +++ config/ia64/predicates.md	(working copy)
> @@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
>  (define_predicate "basereg_operand"
>    (match_operand 0 "register_operand")
>  {
> -  return REG_P (op) && REG_POINTER (op);
> +  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
>  })
>  
> 
> 
> 

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Peter Bergner]

On Thu, 2008-09-18 at 08:40 -0700, Steve Ellcey wrote:
> I tried this patch, it broke on all platforms including x86_64 Linux.
> The x86 build worked but many tests failed with:
> 
> 
> /proj/opensrc/nightly/src/trunk/gcc/testsuite/gcc.c-torture/compile/20000329-1.c: In function 'giop_encode_ulong':
> /proj/opensrc/nightly/src/trunk/gcc/testsuite/gcc.c-torture/compile/20000329-1.c:18: internal compiler error: in gen_reg_rtx, at emit-rtl.c:864

I did say it was untested. :)  The problem is that we cannot call gen_reg_rtx()
during or after reload and gen_reg_rtx_and_attrs() calls gen_reg_rtx(), so I
guess instead of making use of gen_reg_rtx_and_attrs, we should just call
set_reg_attrs_from_value() to set the REG_ATTRS and REG_POINTER...for
non hard regs.  Updated/untested patch below.

Peter


Index: regrename.c
===================================================================
--- regrename.c	(revision 140417)
+++ regrename.c	(working copy)
@@ -356,12 +356,12 @@ do_replace (struct du_chain *chain, int 
   while (chain)
     {
       unsigned int regno = ORIGINAL_REGNO (*chain->loc);
-      struct reg_attrs * attr = REG_ATTRS (*chain->loc);
+      rtx regno_rtx = *chain->loc;
 
-      *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
+      *chain->loc = gen_raw_REG (GET_MODE (regno_rtx), reg);
       if (regno >= FIRST_PSEUDO_REGISTER)
 	ORIGINAL_REGNO (*chain->loc) = regno;
-      REG_ATTRS (*chain->loc) = attr;
+      set_reg_attrs_from_value (*chain->loc, regno_rtx);
       df_insn_rescan (chain->insn);
       chain = chain->next_use;
     }
@@ -1374,7 +1374,7 @@ find_oldest_value_reg (enum reg_class cl
       if (new_rtx)
 	{
 	  ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (reg);
-	  REG_ATTRS (new_rtx) = REG_ATTRS (reg);
+	  set_reg_attrs_from_value (new_rtx, reg);
 	  return new_rtx;
 	}
     }
@@ -1672,7 +1672,7 @@ copyprop_hardreg_forward_1 (basic_block 
 		  if (validate_change (insn, &SET_SRC (set), new_rtx, 0))
 		    {
 		      ORIGINAL_REGNO (new_rtx) = ORIGINAL_REGNO (src);
-		      REG_ATTRS (new_rtx) = REG_ATTRS (src);
+		      set_reg_attrs_from_value (new_rtx, src);
 		      if (dump_file)
 			fprintf (dump_file,
 				 "insn %u: replaced reg %u with %u\n",
Index: config/ia64/predicates.md
===================================================================
--- config/ia64/predicates.md	(revision 140417)
+++ config/ia64/predicates.md	(working copy)
@@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
 (define_predicate "basereg_operand"
   (match_operand 0 "register_operand")
 {
-  return REG_P (op) && REG_POINTER (op);
+  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
 })
 

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Steve Ellcey wrote:
> On Tue, 2008-09-16 at 14:15 -0600, Jeff Law wrote:
>
>   
>> But this shouldn't be necessary.  REG_POINTER is set within the RTX for 
>> the original pseudo and alter_reg (via changing REGNO) preserves the 
>> setting for use post-reload, even though we're looking at a hard register.
>>
>> My recommendation for Steve is the same.  First ensure that REG_POINTER 
>> is set on the appropriate REG within the insn when the insn is created, 
>> then go forward and pinpoint where it disappears as the disappearance is 
>> a bug.
>> Jeff
>>     
>
> Well, it disappears during the rtx register renaming phase and
> I thought I was fixing it by preserving it during register renaming.
> This is where it disappears.   If I add -fno-rename-registers the
> problem goes away.
>   
I went back and found the original message (luckily this is a short 
thread :-)


I think the real problem here is definitely regrename.   We do have to 
be careful about setting REG_POINTER on a hard register, but I think 
we're OK in do_replace as we're always generating a new RTX in that case.

The other two are less clear.  If we choose to only fix do_replace, then 
we might want an indicator in the debugging dumps for the other two 
(find_oldest_value_reg, copyprop_hardreg_forward_1)  when we lose a 
REG_POINTER flag.  It might help diagnose future instances of this problem.

Jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Tue, 2008-09-16 at 15:26 -0600, Jeff Law wrote:
> 
> I think the real problem here is definitely regrename.   We do have to 
> be careful about setting REG_POINTER on a hard register, but I think 
> we're OK in do_replace as we're always generating a new RTX in that case.
> 
> The other two are less clear.  If we choose to only fix do_replace, then 
> we might want an indicator in the debugging dumps for the other two 
> (find_oldest_value_reg, copyprop_hardreg_forward_1)  when we lose a 
> REG_POINTER flag.  It might help diagnose future instances of this problem.
> 
> Jeff

Setting REG_POINTER in do_replace is the only one that is necessary to
fix my bug.  I just changed the others in an attempt to be complete.

Steve Ellcey
sje@cup.hp.com

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Richard Henderson]

Steve Ellcey wrote:
> Setting REG_POINTER in do_replace is the only one that is necessary to
> fix my bug.  I just changed the others in an attempt to be complete.

The change in do_replace is ok.

For the others, where the new rtx is created by maybe_mode_change,
it's unclear which of ORIG or COPY would have the correct value for
REG_POINTER.  It's also unclear why you'd be changing modes like
that for real pointer values.  I suppose if mode==orig_mode and
orig had reg_pointer set, that'd be good enough to set it in the
new rtx.


r~

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Thu, 2008-09-18 at 13:21 -0700, Richard Henderson wrote:
> Steve Ellcey wrote:
> > Setting REG_POINTER in do_replace is the only one that is necessary to
> > fix my bug.  I just changed the others in an attempt to be complete.
> 
> The change in do_replace is ok.
> 
> For the others, where the new rtx is created by maybe_mode_change,
> it's unclear which of ORIG or COPY would have the correct value for
> REG_POINTER.  It's also unclear why you'd be changing modes like
> that for real pointer values.  I suppose if mode==orig_mode and
> orig had reg_pointer set, that'd be good enough to set it in the
> new rtx.
> 
> 
> r~


I tested the following patch where I set REG_POINTER in do_replace and
do not make any other changes.  It fixes
gcc.c-torture/compile/20010102-1.c, gcc.c-torture/compile/20041018-1.c,
and gcc.c-torture/execute/builtins/memset-chk.c on IA64 HP-UX and causes
no regressions.

Is this version of the patch OK for checkin?  I wasn't sure if your
OK comment was an official approval of that part of the patch or not.



2008-08-19  Steve Ellcey  <sje@cup.hp.com>

        * regrename.c (do_replace): Copy REG_POINTER value to new reg.


Index: regrename.c
===================================================================
--- regrename.c (revision 140482)
+++ regrename.c (working copy)
@@ -357,11 +357,13 @@ do_replace (struct du_chain *chain, int 
     {
       unsigned int regno = ORIGINAL_REGNO (*chain->loc);
       struct reg_attrs * attr = REG_ATTRS (*chain->loc);
+      int reg_ptr = REG_POINTER (*chain->loc);
 
       *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
       if (regno >= FIRST_PSEUDO_REGISTER)
        ORIGINAL_REGNO (*chain->loc) = regno;
       REG_ATTRS (*chain->loc) = attr;
+      REG_POINTER (*chain->loc) = reg_ptr;
       df_insn_rescan (chain->insn);
       chain = chain->next_use;

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Steve Ellcey wrote:
> On Thu, 2008-09-18 at 13:21 -0700, Richard Henderson wrote:
>   
>> Steve Ellcey wrote:
>>     
>>> Setting REG_POINTER in do_replace is the only one that is necessary to
>>> fix my bug.  I just changed the others in an attempt to be complete.
>>>       
>> The change in do_replace is ok.
>>
>> For the others, where the new rtx is created by maybe_mode_change,
>> it's unclear which of ORIG or COPY would have the correct value for
>> REG_POINTER.  It's also unclear why you'd be changing modes like
>> that for real pointer values.  I suppose if mode==orig_mode and
>> orig had reg_pointer set, that'd be good enough to set it in the
>> new rtx.
>>
>>
>> r~
>>     
>
>
> I tested the following patch where I set REG_POINTER in do_replace and
> do not make any other changes.  It fixes
> gcc.c-torture/compile/20010102-1.c, gcc.c-torture/compile/20041018-1.c,
> and gcc.c-torture/execute/builtins/memset-chk.c on IA64 HP-UX and causes
> no regressions.
>
> Is this version of the patch OK for checkin?  I wasn't sure if your
> OK comment was an official approval of that part of the patch or not.
>
>
>
> 2008-08-19  Steve Ellcey  <sje@cup.hp.com>
>
>         * regrename.c (do_replace): Copy REG_POINTER value to new reg.
>
>
> Index: regrename.c
> ===================================================================
> --- regrename.c (revision 140482)
> +++ regrename.c (working copy)
> @@ -357,11 +357,13 @@ do_replace (struct du_chain *chain, int 
>      {
>        unsigned int regno = ORIGINAL_REGNO (*chain->loc);
>        struct reg_attrs * attr = REG_ATTRS (*chain->loc);
> +      int reg_ptr = REG_POINTER (*chain->loc);
>  
>        *chain->loc = gen_raw_REG (GET_MODE (*chain->loc), reg);
>        if (regno >= FIRST_PSEUDO_REGISTER)
>         ORIGINAL_REGNO (*chain->loc) = regno;
>        REG_ATTRS (*chain->loc) = attr;
> +      REG_POINTER (*chain->loc) = reg_ptr;
>        df_insn_rescan (chain->insn);
>        chain = chain->next_use;
>
>
>
>   
Is this still pending?  If so, I'll go ahead and explicitly approve 
this, particularly since it's precisely what I thought the patch ought 
to look like after our discussions last week :-)

Jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Tue, 2008-09-23 at 14:38 -0600, Jeff Law wrote:

> > 2008-08-19  Steve Ellcey  <sje@cup.hp.com>
> >
> >         * regrename.c (do_replace): Copy REG_POINTER value to new reg.
> >
> >   
> Is this still pending?  If so, I'll go ahead and explicitly approve 
> this, particularly since it's precisely what I thought the patch ought 
> to look like after our discussions last week :-)
> 
> Jeff

OK, I just checked it in.

Steve Ellcey
sje@cup.hp.com

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

Hi,

I've noticed a 3% performance degradation on CPU2000's vortex (32-bit)
with PEAK flags on PPC (POWER6) after this patch went in. Still
investigating what slowed it down, will post more information soon.

Regards,
Luis

On Tue, 2008-09-23 at 13:44 -0700, Steve Ellcey wrote:
> On Tue, 2008-09-23 at 14:38 -0600, Jeff Law wrote:
> 
> > > 2008-08-19  Steve Ellcey  <sje@cup.hp.com>
> > >
> > >         * regrename.c (do_replace): Copy REG_POINTER value to new reg.
> > >
> > >   
> > Is this still pending?  If so, I'll go ahead and explicitly approve 
> > this, particularly since it's precisely what I thought the patch ought 
> > to look like after our discussions last week :-)
> > 
> > Jeff
> 
> OK, I just checked it in.
> 
> Steve Ellcey
> sje@cup.hp.com
> 

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Luis Machado wrote:
> Hi,
>
> I've noticed a 3% performance degradation on CPU2000's vortex (32-bit)
> with PEAK flags on PPC (POWER6) after this patch went in. Still
> investigating what slowed it down, will post more information soon.
>   
I'd be hard pressed to see how this patch could cause any kind of 
performance regression since it's merely propagating information to the 
backend that was getting lost in regrename and I don't see that the ppc 
uses REG_POINTER in its backend.

Jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Andrew Pinski]

On Fri, Oct 3, 2008 at 5:40 PM, Jeff Law <law@redhat.com> wrote:
> I'd be hard pressed to see how this patch could cause any kind of
> performance regression since it's merely propagating information to the
> backend that was getting lost in regrename and I don't see that the ppc uses
> REG_POINTER in its backend.

PPC does not use it but the way lwx is used is [base+index] which
should really be done that way, otherwise it will have some stalls in
some cases (on Power 6).

Thanks,
Andrew Pinski

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

On Fri, 2008-10-03 at 17:46 -0700, Andrew Pinski wrote: 
> On Fri, Oct 3, 2008 at 5:40 PM, Jeff Law <law@redhat.com> wrote:
> > I'd be hard pressed to see how this patch could cause any kind of
> > performance regression since it's merely propagating information to
> the
> > backend that was getting lost in regrename and I don't see that the
> ppc uses
> > REG_POINTER in its backend.
> 
> PPC does not use it but the way lwx is used is [base+index] which
> should really be done that way, otherwise it will have some stalls in
> some cases (on Power 6).
> 
> Thanks,
> Andrew Pinski

After further investigation, there is some light to this problem.

There appears to be a bad interaction between REG_POINTER and RTL alias
analysis, as we see below.

The epilogue for these two functions from vortex (Mem_GetAddr,
Mem_GetWord) are the following, for both revisions:

Mem_GetAddr (revision 140615):

.L3:
        mr 3,29
        mr 4,30
        bl Ut_PrintErr
        lwz 7,36(1)
        lwz 26,8(1)
*        lis 8,Test@ha
        lwz 27,12(1)
        lwz 28,16(1)
        mtlr 7
        mr 0,3
        lwz 29,20(1)
        lwz 30,24(1)
        mr 3,0
        lwz 31,28(1)
*        stw 0,Test@l(8)
        addi 1,1,32
        blr

Mem_GetAddr (revision 140616):

.L3:
        mr 3,29
        mr 4,30
        bl Ut_PrintErr
*        lis 8,Test@ha
        mr 0,3
*        stw 0,Test@l(8)
        mr 3,0
        lwz 7,36(1)
        lwz 26,8(1)
        lwz 27,12(1)
        lwz 28,16(1)
        mtlr 7
        lwz 29,20(1)
        lwz 30,24(1)
        lwz 31,28(1)
        addi 1,1,32
        blr


Mem_GetWord (revisions 140615 and 140616):

.L8:
        lwz 5,0(31)
        li 0,1
        cmpwi 6,5,0
        bne- 6,.L9
        lwz 10,36(1)
        lwz 26,8(1)
*        lis 11,Test@ha
        mr 3,0
        lwz 27,12(1)
        lwz 28,16(1)
        mtlr 10
*        stw 0,Test@l(11)
        lwz 29,20(1)
        lwz 30,24(1)
        lwz 31,28(1)
        addi 1,1,32
        blr

Clearly, the code generation for Mem_GetWord is unaffected by the
changes from revision 140616.

The problem happens with Mem_GetAddr/revision 140616. We can see that
"lis 8,Test@ha" and "stw 0,Test@l(8)" are very close to each other,
leading to possible stalls in the pipeline, which doesn't happen with
the code generated by revision 140615. 

The positioning of "lis" and "stw" we see in revision 140616 is due to a
number of dependencies created wrt the following lwz instructions, so
the "stw" needs to execute before that, avoiding the possibility to move
"stw" further down.

So we're left with the question of why these dependencies are being
created.

Digging further, revision 140616 enabled some registers to store a value
meaning its contents are, in fact, a pointer, and we access this data
with REG_POINTER(...).

Looking at this specific code inside alias.c:find_base_term:

        if (REG_P (tmp1) && REG_POINTER (tmp1))
          return find_base_term (tmp1);

        if (REG_P (tmp2) && REG_POINTER (tmp2))
          return find_base_term (tmp2);

Since up until revision 140615 we didn't store the specific information
mentioned above, we just flew by these conditions and returned either
tmp1 or tmp2, not going into the recursive call.

With revision 140616, we actually have the pointer-in-reg information,
and thus we go into the recursive call to either "find_base_term (tmp1)"
or "find_base_term (tmp2)".

This should work as expected, as it really does for Mem_GetWord, but for
Mem_GetAddr, the recursive call to "find_base_term (tmp1)" returns NULL.

This leads us back to "init_alias_analysis", where we actually fill up
the reg_base_value vector that is used, inside find_base_term, to return
the base term we want.

Keeping it short, the 8th entry in that vector, that should've contained
the correct base term, is blank due to the 8th register being defined
and set before the "stw" instruction in the epilogue. This specific
situation led to all the others, including the performance degradation.

This also relates to the -fno-strict-aliasing flag, which sets aliases
to 0 and we need to do extra work to sort out the conflicting pieces.
Without -fno-strict-aliasing, the code is generated correctly.

So, it seems the RTL alias analysis framework in GCC is not designed to
handle REG_POINTER" on hard registers. I would like to suggest that we
return to Peter Bergner's proposed solution.

Regards,
Luis

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Luis Machado wrote:
> On Fri, 2008-10-03 at 17:46 -0700, Andrew Pinski wrote: 
>   
>> On Fri, Oct 3, 2008 at 5:40 PM, Jeff Law <law@redhat.com> wrote:
>>     
>>> I'd be hard pressed to see how this patch could cause any kind of
>>> performance regression since it's merely propagating information to
>>>       
>> the
>>     
>>> backend that was getting lost in regrename and I don't see that the
>>>       
>> ppc uses
>>     
>>> REG_POINTER in its backend.
>>>       
>> PPC does not use it but the way lwx is used is [base+index] which
>> should really be done that way, otherwise it will have some stalls in
>> some cases (on Power 6).
>>
>> Thanks,
>> Andrew Pinski
>>     
>
> After further investigation, there is some light to this problem.
>
> There appears to be a bad interaction between REG_POINTER and RTL alias
> analysis, as we see below.
>
> The epilogue for these two functions from vortex (Mem_GetAddr,
> Mem_GetWord) are the following, for both revisions:
>
> Mem_GetAddr (revision 140615):
>
> .L3:
>         mr 3,29
>         mr 4,30
>         bl Ut_PrintErr
>         lwz 7,36(1)
>         lwz 26,8(1)
> *        lis 8,Test@ha
>         lwz 27,12(1)
>         lwz 28,16(1)
>         mtlr 7
>         mr 0,3
>         lwz 29,20(1)
>         lwz 30,24(1)
>         mr 3,0
>         lwz 31,28(1)
> *        stw 0,Test@l(8)
>         addi 1,1,32
>         blr
>
> Mem_GetAddr (revision 140616):
>
> .L3:
>         mr 3,29
>         mr 4,30
>         bl Ut_PrintErr
> *        lis 8,Test@ha
>         mr 0,3
> *        stw 0,Test@l(8)
>         mr 3,0
>         lwz 7,36(1)
>         lwz 26,8(1)
>         lwz 27,12(1)
>         lwz 28,16(1)
>         mtlr 7
>         lwz 29,20(1)
>         lwz 30,24(1)
>         lwz 31,28(1)
>         addi 1,1,32
>         blr
>
>
> Mem_GetWord (revisions 140615 and 140616):
>
> .L8:
>         lwz 5,0(31)
>         li 0,1
>         cmpwi 6,5,0
>         bne- 6,.L9
>         lwz 10,36(1)
>         lwz 26,8(1)
> *        lis 11,Test@ha
>         mr 3,0
>         lwz 27,12(1)
>         lwz 28,16(1)
>         mtlr 10
> *        stw 0,Test@l(11)
>         lwz 29,20(1)
>         lwz 30,24(1)
>         lwz 31,28(1)
>         addi 1,1,32
>         blr
>
> Clearly, the code generation for Mem_GetWord is unaffected by the
> changes from revision 140616.
>
> The problem happens with Mem_GetAddr/revision 140616. We can see that
> "lis 8,Test@ha" and "stw 0,Test@l(8)" are very close to each other,
> leading to possible stalls in the pipeline, which doesn't happen with
> the code generated by revision 140615. 
>
> The positioning of "lis" and "stw" we see in revision 140616 is due to a
> number of dependencies created wrt the following lwz instructions, so
> the "stw" needs to execute before that, avoiding the possibility to move
> "stw" further down.
>
> So we're left with the question of why these dependencies are being
> created.
>
> Digging further, revision 140616 enabled some registers to store a value
> meaning its contents are, in fact, a pointer, and we access this data
> with REG_POINTER(...).
>
> Looking at this specific code inside alias.c:find_base_term:
>
>         if (REG_P (tmp1) && REG_POINTER (tmp1))
>           return find_base_term (tmp1);
>
>         if (REG_P (tmp2) && REG_POINTER (tmp2))
>           return find_base_term (tmp2);
>
> Since up until revision 140615 we didn't store the specific information
> mentioned above, we just flew by these conditions and returned either
> tmp1 or tmp2, not going into the recursive call.
>
> With revision 140616, we actually have the pointer-in-reg information,
> and thus we go into the recursive call to either "find_base_term (tmp1)"
> or "find_base_term (tmp2)".
>
> This should work as expected, as it really does for Mem_GetWord, but for
> Mem_GetAddr, the recursive call to "find_base_term (tmp1)" returns NULL.
>
> This leads us back to "init_alias_analysis", where we actually fill up
> the reg_base_value vector that is used, inside find_base_term, to return
> the base term we want.
>
> Keeping it short, the 8th entry in that vector, that should've contained
> the correct base term, is blank due to the 8th register being defined
> and set before the "stw" instruction in the epilogue. This specific
> situation led to all the others, including the performance degradation.
>
> This also relates to the -fno-strict-aliasing flag, which sets aliases
> to 0 and we need to do extra work to sort out the conflicting pieces.
> Without -fno-strict-aliasing, the code is generated correctly.
>
> So, it seems the RTL alias analysis framework in GCC is not designed to
> handle REG_POINTER" on hard registers. I would like to suggest that we
> return to Peter Bergner's proposed solution.
>   
Then it's the RTL alias analysis that needs to be fixed   -- Steve's 
solution to regrename is the correct solution. 

Jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

On Thu, 2008-10-16 at 12:45 -0600, Jeff Law wrote:
> Luis Machado wrote:
> > On Fri, 2008-10-03 at 17:46 -0700, Andrew Pinski wrote: 
> >   
> >> On Fri, Oct 3, 2008 at 5:40 PM, Jeff Law <law@redhat.com> wrote:
> >>     
> >>> I'd be hard pressed to see how this patch could cause any kind of
> >>> performance regression since it's merely propagating information to
> >>>       
> >> the
> >>     
> >>> backend that was getting lost in regrename and I don't see that the
> >>>       
> >> ppc uses
> >>     
> >>> REG_POINTER in its backend.
> >>>       
> >> PPC does not use it but the way lwx is used is [base+index] which
> >> should really be done that way, otherwise it will have some stalls in
> >> some cases (on Power 6).
> >>
> >> Thanks,
> >> Andrew Pinski
> >>     
> >
> > After further investigation, there is some light to this problem.
> >
> > There appears to be a bad interaction between REG_POINTER and RTL alias
> > analysis, as we see below.
> >
> > The epilogue for these two functions from vortex (Mem_GetAddr,
> > Mem_GetWord) are the following, for both revisions:
> >
> > Mem_GetAddr (revision 140615):
> >
> > .L3:
> >         mr 3,29
> >         mr 4,30
> >         bl Ut_PrintErr
> >         lwz 7,36(1)
> >         lwz 26,8(1)
> > *        lis 8,Test@ha
> >         lwz 27,12(1)
> >         lwz 28,16(1)
> >         mtlr 7
> >         mr 0,3
> >         lwz 29,20(1)
> >         lwz 30,24(1)
> >         mr 3,0
> >         lwz 31,28(1)
> > *        stw 0,Test@l(8)
> >         addi 1,1,32
> >         blr
> >
> > Mem_GetAddr (revision 140616):
> >
> > .L3:
> >         mr 3,29
> >         mr 4,30
> >         bl Ut_PrintErr
> > *        lis 8,Test@ha
> >         mr 0,3
> > *        stw 0,Test@l(8)
> >         mr 3,0
> >         lwz 7,36(1)
> >         lwz 26,8(1)
> >         lwz 27,12(1)
> >         lwz 28,16(1)
> >         mtlr 7
> >         lwz 29,20(1)
> >         lwz 30,24(1)
> >         lwz 31,28(1)
> >         addi 1,1,32
> >         blr
> >
> >
> > Mem_GetWord (revisions 140615 and 140616):
> >
> > .L8:
> >         lwz 5,0(31)
> >         li 0,1
> >         cmpwi 6,5,0
> >         bne- 6,.L9
> >         lwz 10,36(1)
> >         lwz 26,8(1)
> > *        lis 11,Test@ha
> >         mr 3,0
> >         lwz 27,12(1)
> >         lwz 28,16(1)
> >         mtlr 10
> > *        stw 0,Test@l(11)
> >         lwz 29,20(1)
> >         lwz 30,24(1)
> >         lwz 31,28(1)
> >         addi 1,1,32
> >         blr
> >
> > Clearly, the code generation for Mem_GetWord is unaffected by the
> > changes from revision 140616.
> >
> > The problem happens with Mem_GetAddr/revision 140616. We can see that
> > "lis 8,Test@ha" and "stw 0,Test@l(8)" are very close to each other,
> > leading to possible stalls in the pipeline, which doesn't happen with
> > the code generated by revision 140615. 
> >
> > The positioning of "lis" and "stw" we see in revision 140616 is due to a
> > number of dependencies created wrt the following lwz instructions, so
> > the "stw" needs to execute before that, avoiding the possibility to move
> > "stw" further down.
> >
> > So we're left with the question of why these dependencies are being
> > created.
> >
> > Digging further, revision 140616 enabled some registers to store a value
> > meaning its contents are, in fact, a pointer, and we access this data
> > with REG_POINTER(...).
> >
> > Looking at this specific code inside alias.c:find_base_term:
> >
> >         if (REG_P (tmp1) && REG_POINTER (tmp1))
> >           return find_base_term (tmp1);
> >
> >         if (REG_P (tmp2) && REG_POINTER (tmp2))
> >           return find_base_term (tmp2);
> >
> > Since up until revision 140615 we didn't store the specific information
> > mentioned above, we just flew by these conditions and returned either
> > tmp1 or tmp2, not going into the recursive call.
> >
> > With revision 140616, we actually have the pointer-in-reg information,
> > and thus we go into the recursive call to either "find_base_term (tmp1)"
> > or "find_base_term (tmp2)".
> >
> > This should work as expected, as it really does for Mem_GetWord, but for
> > Mem_GetAddr, the recursive call to "find_base_term (tmp1)" returns NULL.
> >
> > This leads us back to "init_alias_analysis", where we actually fill up
> > the reg_base_value vector that is used, inside find_base_term, to return
> > the base term we want.
> >
> > Keeping it short, the 8th entry in that vector, that should've contained
> > the correct base term, is blank due to the 8th register being defined
> > and set before the "stw" instruction in the epilogue. This specific
> > situation led to all the others, including the performance degradation.
> >
> > This also relates to the -fno-strict-aliasing flag, which sets aliases
> > to 0 and we need to do extra work to sort out the conflicting pieces.
> > Without -fno-strict-aliasing, the code is generated correctly.
> >
> > So, it seems the RTL alias analysis framework in GCC is not designed to
> > handle REG_POINTER" on hard registers. I would like to suggest that we
> > return to Peter Bergner's proposed solution.
> >   
> Then it's the RTL alias analysis that needs to be fixed   -- Steve's 
> solution to regrename is the correct solution. 
> 
> Jeff

Considering that the current RTL analysis code was not thought to handle
this scenario, wouldn't a proper fix to the RTL code together with the
REG_POINTER solution be appropriate?

Steve?

Regards,
Luis

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Thu, 2008-10-30 at 10:41 -0200, Luis Machado wrote:

> > Then it's the RTL alias analysis that needs to be fixed   -- Steve's 
> > solution to regrename is the correct solution. 
> > 
> > Jeff
> 
> Considering that the current RTL analysis code was not thought to handle
> this scenario, wouldn't a proper fix to the RTL code together with the
> REG_POINTER solution be appropriate?
> 
> Steve?
> 
> Regards,
> Luis

I am looking into this some more.  For some reason, if I remove my patch
from the ToT sources I cannot reproduce the IA64 bug anymore.  I want to
find out what change made the bug stop triggering (in the absence of my
patch).  I don't want to just remove my patch but it might make sense to
change regrename to only set the attributes (including pointer) for
non-hard registers if that fixes your problem and doesn't cause a
regression for me.

Steve Ellcey
sje@cup.hp.com

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Peter Bergner]

On Thu, 2008-10-30 at 10:52 -0700, Steve Ellcey wrote:
> I don't want to just remove my patch but it might make sense to
> change regrename to only set the attributes (including pointer) for
> non-hard registers if that fixes your problem and doesn't cause a
> regression for me.

I actually have already asked Luis to test that.  My guess is that
is what we eventually will end up wanting to go with, but you might
then need to use the patch I posted for ia64/predicates.md, namely:

Index: config/ia64/predicates.md
===================================================================
--- config/ia64/predicates.md   (revision 140417)
+++ config/ia64/predicates.md   (working copy)
@@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
 (define_predicate "basereg_operand"
   (match_operand 0 "register_operand")
 {
-  return REG_P (op) && REG_POINTER (op);
+  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
 })

If you recall, that did solve the ICE you were hitting.  The rerename.c
change was just handling a case that looked like should be handled, but
wasn't necessary for fixing your ICE.

Peter

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Peter Bergner wrote:
> On Thu, 2008-10-30 at 10:52 -0700, Steve Ellcey wrote:
>   
>> I don't want to just remove my patch but it might make sense to
>> change regrename to only set the attributes (including pointer) for
>> non-hard registers if that fixes your problem and doesn't cause a
>> regression for me.
>>     
>
> I actually have already asked Luis to test that.  My guess is that
> is what we eventually will end up wanting to go with, but you might
> then need to use the patch I posted for ia64/predicates.md, namely:
>
> Index: config/ia64/predicates.md
> ===================================================================
> --- config/ia64/predicates.md   (revision 140417)
> +++ config/ia64/predicates.md   (working copy)
> @@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
>  (define_predicate "basereg_operand"
>    (match_operand 0 "register_operand")
>  {
> -  return REG_P (op) && REG_POINTER (op);
> +  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
>  })
>
> If you recall, that did solve the ICE you were hitting.  The rerename.c
> change was just handling a case that looked like should be handled, but
> wasn't necessary for fixing your IC
>   
We shouldn't be hacking up backends to deal with this problem, it's 
entirely the wrong approach.


jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Steve Ellcey]

On Thu, 2008-10-30 at 12:38 -0600, Jeff Law wrote:

> We shouldn't be hacking up backends to deal with this problem, it's 
> entirely the wrong approach.
> 
> 
> jeff

So I looked at why I wasn't seeing this in ToT with -O3 and found that
it was due to the selective scheduler checkin.  However, If I use -O2
-funroll-all-loops I can still reproduce this at ToT after removing my
patch or after changing it to not affect hard registers.

I am thinking that the right approach is to change regrename somewhere
so that a hard register with REG_POINTER set to true and a hard register
with REG_POINTER set to false are not allowed in a rename (at least not
on IA64 HP-UX in ILP32 mode).  HARD_REGNO_RENAME_OK seems like the
natural place to do this but it takes register numbers as arguments and
I don't know how to find the REG_POINTER value for a hard register from
just the register number (as opposed to having an actual register RTL).

Steve Ellcey
sje@cup.hp.com

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Steve Ellcey wrote:
> On Thu, 2008-10-30 at 12:38 -0600, Jeff Law wrote:
>
>   
>> We shouldn't be hacking up backends to deal with this problem, it's 
>> entirely the wrong approach.
>>
>>
>> jeff
>>     
>
> So I looked at why I wasn't seeing this in ToT with -O3 and found that
> it was due to the selective scheduler checkin.  However, If I use -O2
> -funroll-all-loops I can still reproduce this at ToT after removing my
> patch or after changing it to not affect hard registers.
>   
I figured it was simply a matter of some pass changing the code and 
hence hiding the problem.

> I am thinking that the right approach is to change regrename somewhere
> so that a hard register with REG_POINTER set to true and a hard register
> with REG_POINTER set to false are not allowed in a rename (at least not
> on IA64 HP-UX in ILP32 mode).  HARD_REGNO_RENAME_OK seems like the
> natural place to do this but it takes register numbers as arguments and
> I don't know how to find the REG_POINTER value for a hard register from
> just the register number (as opposed to having an actual register RTL).
>
>   
Given a hard register #, you can't map back to its usage without 
scanning the RTL as hard registers are not shared, particularly after 
register allocation.

I really think we need to go back and look at handling this better in 
alias.c -- that's really the only thing that's being complained about.  
If we can't find a way to fix alias.c, then and only then should we look 
at workarounds such as avoiding renaming on values with REG_POINTER set 
and similar hacks.

Jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jakub Jelinek]

On Thu, Oct 30, 2008 at 12:38:09PM -0600, Jeff Law wrote:
> Peter Bergner wrote:
>> On Thu, 2008-10-30 at 10:52 -0700, Steve Ellcey wrote:
>> --- config/ia64/predicates.md   (revision 140417)
>> +++ config/ia64/predicates.md   (working copy)
>> @@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
>>  (define_predicate "basereg_operand"
>>    (match_operand 0 "register_operand")
>>  {
>> -  return REG_P (op) && REG_POINTER (op);
>> +  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
>>  })
>>
>> If you recall, that did solve the ICE you were hitting.  The rerename.c
>> change was just handling a case that looked like should be handled, but
>> wasn't necessary for fixing your IC
>>   
> We shouldn't be hacking up backends to deal with this problem, it's  
> entirely the wrong approach.

Why?  REG_POINTER can't (reliably) work on hard registers, because through
the life of one function one hard register might contain in some places
a pointer and in others non-pointer.  While pseudos should be always either
pointer or non-pointer.  IMHO (almost) all uses of REG_POINTER should
be changed to work on ORIGINAL_REGNO.

	Jakub

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Peter Bergner]

On Thu, 2008-10-30 at 12:38 -0600, Jeff Law wrote:
> Peter Bergner wrote:
[snip]
> > Index: config/ia64/predicates.md
> > ===================================================================
> > --- config/ia64/predicates.md   (revision 140417)
> > +++ config/ia64/predicates.md   (working copy)
> > @@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
> >  (define_predicate "basereg_operand"
> >    (match_operand 0 "register_operand")
> >  {
> > -  return REG_P (op) && REG_POINTER (op);
> > +  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
> >  })
> >
> We shouldn't be hacking up backends to deal with this problem, it's 
> entirely the wrong approach.

I have to agree with Jakub, why not?  As Jakub mentioned, REG_POINTER
cannot make sense on a hard register since it can contain many unrelated
values, some being pointers and some not.  We actually already hit and
fixed a problem where we were setting the REG_POINTER attribute on a
hard register:

    http://gcc.gnu.org/ml/gcc-patches/2008-06/msg01425.html

Or are you saying this predicate should never be called with a
hard register?

Peter

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

Hi folks,

On Fri, 2008-10-31 at 15:07 -0500, Peter Bergner wrote:
> On Thu, 2008-10-30 at 12:38 -0600, Jeff Law wrote:
> > Peter Bergner wrote:
> [snip]
> > > Index: config/ia64/predicates.md
> > > ===================================================================
> > > --- config/ia64/predicates.md   (revision 140417)
> > > +++ config/ia64/predicates.md   (working copy)
> > > @@ -585,6 +585,6 @@ (define_predicate "ar_pfs_reg_operand"
> > >  (define_predicate "basereg_operand"
> > >    (match_operand 0 "register_operand")
> > >  {
> > > -  return REG_P (op) && REG_POINTER (op);
> > > +  return REG_P (op) && REG_POINTER (regno_reg_rtx[ORIGINAL_REGNO (op)]);
> > >  })
> > >
> > We shouldn't be hacking up backends to deal with this problem, it's 
> > entirely the wrong approach.
> 
> I have to agree with Jakub, why not?  As Jakub mentioned, REG_POINTER
> cannot make sense on a hard register since it can contain many unrelated
> values, some being pointers and some not.  We actually already hit and
> fixed a problem where we were setting the REG_POINTER attribute on a
> hard register:
> 
>     http://gcc.gnu.org/ml/gcc-patches/2008-06/msg01425.html
> 
> Or are you saying this predicate should never be called with a
> hard register?
> 
> Peter

Changing the patch to set REG_POINTER only to non-hard registers fixed
the performance regression we saw on SPEC2k (vortex).

Regards,
Luis

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jakub Jelinek]

On Fri, Oct 31, 2008 at 03:07:28PM -0500, Peter Bergner wrote:
> On Thu, 2008-10-30 at 12:38 -0600, Jeff Law wrote:
> > We shouldn't be hacking up backends to deal with this problem, it's 
> > entirely the wrong approach.
> 
> I have to agree with Jakub, why not?  As Jakub mentioned, REG_POINTER
> cannot make sense on a hard register since it can contain many unrelated
> values, some being pointers and some not.  We actually already hit and
> fixed a problem where we were setting the REG_POINTER attribute on a
> hard register:
> 
>     http://gcc.gnu.org/ml/gcc-patches/2008-06/msg01425.html
> 
> Or are you saying this predicate should never be called with a
> hard register?

Or change REG_POINTER, so that it hops through ORIGINAL_REGNO automatically
or ORIGINAL_REGNO is set?  Guess we'd need another macro for setting
REG_POINTER in that case.

	Jakub

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Luis Machado wrote:
> On Thu, 2008-10-16 at 12:45 -0600, Jeff Law wrote:
>   
>> Luis Machado wrote:
>>     
>>> On Fri, 2008-10-03 at 17:46 -0700, Andrew Pinski wrote: 
>>>   
>>>       
>>>> On Fri, Oct 3, 2008 at 5:40 PM, Jeff Law <law@redhat.com> wrote:
>>>>     
>>>>         
>>>>> I'd be hard pressed to see how this patch could cause any kind of
>>>>> performance regression since it's merely propagating information to
>>>>>       
>>>>>           
>>>> the
>>>>     
>>>>         
>>>>> backend that was getting lost in regrename and I don't see that the
>>>>>       
>>>>>           
>>>> ppc uses
>>>>     
>>>>         
>>>>> REG_POINTER in its backend.
>>>>>       
>>>>>           
>>>> PPC does not use it but the way lwx is used is [base+index] which
>>>> should really be done that way, otherwise it will have some stalls in
>>>> some cases (on Power 6).
>>>>
>>>> Thanks,
>>>> Andrew Pinski
>>>>     
>>>>         
>>> After further investigation, there is some light to this problem.
>>>
>>> There appears to be a bad interaction between REG_POINTER and RTL alias
>>> analysis, as we see below.
>>>
>>> The epilogue for these two functions from vortex (Mem_GetAddr,
>>> Mem_GetWord) are the following, for both revisions:
>>>
>>> Mem_GetAddr (revision 140615):
>>>
>>> .L3:
>>>         mr 3,29
>>>         mr 4,30
>>>         bl Ut_PrintErr
>>>         lwz 7,36(1)
>>>         lwz 26,8(1)
>>> *        lis 8,Test@ha
>>>         lwz 27,12(1)
>>>         lwz 28,16(1)
>>>         mtlr 7
>>>         mr 0,3
>>>         lwz 29,20(1)
>>>         lwz 30,24(1)
>>>         mr 3,0
>>>         lwz 31,28(1)
>>> *        stw 0,Test@l(8)
>>>         addi 1,1,32
>>>         blr
>>>
>>> Mem_GetAddr (revision 140616):
>>>
>>> .L3:
>>>         mr 3,29
>>>         mr 4,30
>>>         bl Ut_PrintErr
>>> *        lis 8,Test@ha
>>>         mr 0,3
>>> *        stw 0,Test@l(8)
>>>         mr 3,0
>>>         lwz 7,36(1)
>>>         lwz 26,8(1)
>>>         lwz 27,12(1)
>>>         lwz 28,16(1)
>>>         mtlr 7
>>>         lwz 29,20(1)
>>>         lwz 30,24(1)
>>>         lwz 31,28(1)
>>>         addi 1,1,32
>>>         blr
>>>
>>>
>>> Mem_GetWord (revisions 140615 and 140616):
>>>
>>> .L8:
>>>         lwz 5,0(31)
>>>         li 0,1
>>>         cmpwi 6,5,0
>>>         bne- 6,.L9
>>>         lwz 10,36(1)
>>>         lwz 26,8(1)
>>> *        lis 11,Test@ha
>>>         mr 3,0
>>>         lwz 27,12(1)
>>>         lwz 28,16(1)
>>>         mtlr 10
>>> *        stw 0,Test@l(11)
>>>         lwz 29,20(1)
>>>         lwz 30,24(1)
>>>         lwz 31,28(1)
>>>         addi 1,1,32
>>>         blr
>>>
>>> Clearly, the code generation for Mem_GetWord is unaffected by the
>>> changes from revision 140616.
>>>
>>> The problem happens with Mem_GetAddr/revision 140616. We can see that
>>> "lis 8,Test@ha" and "stw 0,Test@l(8)" are very close to each other,
>>> leading to possible stalls in the pipeline, which doesn't happen with
>>> the code generated by revision 140615. 
>>>
>>> The positioning of "lis" and "stw" we see in revision 140616 is due to a
>>> number of dependencies created wrt the following lwz instructions, so
>>> the "stw" needs to execute before that, avoiding the possibility to move
>>> "stw" further down.
>>>
>>> So we're left with the question of why these dependencies are being
>>> created.
>>>
>>> Digging further, revision 140616 enabled some registers to store a value
>>> meaning its contents are, in fact, a pointer, and we access this data
>>> with REG_POINTER(...).
>>>
>>> Looking at this specific code inside alias.c:find_base_term:
>>>
>>>         if (REG_P (tmp1) && REG_POINTER (tmp1))
>>>           return find_base_term (tmp1);
>>>
>>>         if (REG_P (tmp2) && REG_POINTER (tmp2))
>>>           return find_base_term (tmp2);
>>>
>>> Since up until revision 140615 we didn't store the specific information
>>> mentioned above, we just flew by these conditions and returned either
>>> tmp1 or tmp2, not going into the recursive call.
>>>
>>> With revision 140616, we actually have the pointer-in-reg information,
>>> and thus we go into the recursive call to either "find_base_term (tmp1)"
>>> or "find_base_term (tmp2)".
>>>
>>> This should work as expected, as it really does for Mem_GetWord, but for
>>> Mem_GetAddr, the recursive call to "find_base_term (tmp1)" returns NULL.
>>>
>>> This leads us back to "init_alias_analysis", where we actually fill up
>>> the reg_base_value vector that is used, inside find_base_term, to return
>>> the base term we want.
>>>
>>> Keeping it short, the 8th entry in that vector, that should've contained
>>> the correct base term, is blank due to the 8th register being defined
>>> and set before the "stw" instruction in the epilogue. This specific
>>> situation led to all the others, including the performance degradation.
>>>
>>> This also relates to the -fno-strict-aliasing flag, which sets aliases
>>> to 0 and we need to do extra work to sort out the conflicting pieces.
>>> Without -fno-strict-aliasing, the code is generated correctly.
>>>
>>> So, it seems the RTL alias analysis framework in GCC is not designed to
>>> handle REG_POINTER" on hard registers. I would like to suggest that we
>>> return to Peter Bergner's proposed solution.
>>>   
>>>       
>> Then it's the RTL alias analysis that needs to be fixed   -- Steve's 
>> solution to regrename is the correct solution. 
>>
>> Jeff
>>     
>
> Considering that the current RTL analysis code was not thought to handle
> this scenario, wouldn't a proper fix to the RTL code together with the
> REG_POINTER solution be appropriate?
>   
It would actually be helpful to see the RTL in question to help guide 
suggestions for how to fix the aliasing code.   For the code to have the 
effect you've described, I think the RTL must have had a form like

(minus/plus/lo_sum (reg) (symbol_ref))

Where (reg) is a hard register marked as a pointer.

If that's the case, then it seems to me you can get the behaviour you 
want with a fairly simple change.

if (REG_P (tmp1) && REG_POINTER (tmp1))
  {
    rtx base = find_base_term (tmp1)
    if (base)
      return base;
  }


And similarly for tmp2.

Basically this would cause us to fall through those two tests and not 
return NULL too early in the case you've described.  We'd then fall into 
the code immediately after which will see if the base term is a named 
object or special address and if so, it'll use the named 
objected/special address.

Can you try that and see if it helps.  If it doesn't, then you're going 
to need to provide more information (RTL) causing the problematical 
behaviour.

jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[David Edelsohn]

Jeff,

The RTL is a pair of (high (symbol_ref)) and (lo_sum (reg) (symbol_ref)).

alias.c:find_base_value() has a special case for lo_sum:

    case LO_SUM:
      /* The standard form is (lo_sum reg sym) so look only at the
         second operand.  */
      return find_base_value (XEXP (src, 1));

However, find_base_term() throws lo_sum in with PLUS and MINUS,
which prefers the REG.  find_base_term() appears to expect that the
symbol_ref from the HIGH will be recorded with the REG and picked
up by the LO_SUM from that source.

find_base_term() either could check for non-zero base before returning
in the lo_sum/plus/minus case statement or could handle lo_sum
explicitly, like find_base_value().

David

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

David Edelsohn wrote:
> Jeff,
>
> The RTL is a pair of (high (symbol_ref)) and (lo_sum (reg) (symbol_ref)).
>   
Presumably at the point in question we're looking at the lo_sum insn, right?

> alias.c:find_base_value() has a special case for lo_sum:
>
>     case LO_SUM:
>       /* The standard form is (lo_sum reg sym) so look only at the
>          second operand.  */
>       return find_base_value (XEXP (src, 1));
>
> However, find_base_term() throws lo_sum in with PLUS and MINUS,
> which prefers the REG.  find_base_term() appears to expect that the
> symbol_ref from the HIGH will be recorded with the REG and picked
> up by the LO_SUM from that source.
>
> find_base_term() either could check for non-zero base before returning
> in the lo_sum/plus/minus case statement or could handle lo_sum
> explicitly, like find_base_value().
>   
I think my suggestion will accomplish the same thing -- and it makes 
logical sense too -- if we can't determine something from the pointer 
reg, then we look at other info, such as the symbol_ref within a lo_sum.


Jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[David Edelsohn]

On Thu, Nov 6, 2008 at 4:31 PM, Jeff Law <law@redhat.com> wrote:

> Presumably at the point in question we're looking at the lo_sum insn, right?

Yes.


>> find_base_term() either could check for non-zero base before returning
>> in the lo_sum/plus/minus case statement or could handle lo_sum
>> explicitly, like find_base_value().
>
> I think my suggestion will accomplish the same thing -- and it makes logical
> sense too -- if we can't determine something from the pointer reg, then we
> look at other info, such as the symbol_ref within a lo_sum.

Yes, it will accomplish the same thing for this particular case.

Currently find_base_value and find_base_term differ in their handling
of LO_SUM.  My question is whether this fix should maintain the difference
in algorithms.  Also, RTL alias analysis is very fragile and your suggestion
may produce more accurate information in other situations, which could
expose other latent bugs or unexpected behavior.

Luis is going to test both approaches.

Thanks, David

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

David Edelsohn wrote:
>
>   
>>> find_base_term() either could check for non-zero base before returning
>>> in the lo_sum/plus/minus case statement or could handle lo_sum
>>> explicitly, like find_base_value().
>>>       
>> I think my suggestion will accomplish the same thing -- and it makes logical
>> sense too -- if we can't determine something from the pointer reg, then we
>> look at other info, such as the symbol_ref within a lo_sum.
>>     
>
> Yes, it will accomplish the same thing for this particular case.
>
> Currently find_base_value and find_base_term differ in their handling
> of LO_SUM.  My question is whether this fix should maintain the difference
> in algorithms. 
I suspect the differences are unintentional and  one could easily argue 
that find_base_value's handling of LO_SUM is better since LO_SUM has 
pretty well defined semantics.  One could also argue that both functions 
shouldn't be so eager to return the results of the recursive call when 
the recursive call returns NULL.

>  Also, RTL alias analysis is very fragile and your suggestion
> may produce more accurate information in other situations, which could
> expose other latent bugs or unexpected behavior.
>   
Part of the intent was to get the more accurate info in the presence of 
registers marked with REG_POINTER.  While it could expose latent bugs, I 
think it's the right thing to do -- though we might look for a change 
with a smaller impact for the upcoming release since I think we're in 
stage3.

jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

Hi,

Both approaches fix the regression on vortex with no noticeable
performance degradations on 32-bit. I'll check the numbers for 64-bit to
make sure we're OK there as well. I'll also check the testsuite results
with both approaches.

Regards,
Luis


On Thu, 2008-11-06 at 16:16 -0700, Jeff Law wrote:
> David Edelsohn wrote:
> >
> >   
> >>> find_base_term() either could check for non-zero base before returning
> >>> in the lo_sum/plus/minus case statement or could handle lo_sum
> >>> explicitly, like find_base_value().
> >>>       
> >> I think my suggestion will accomplish the same thing -- and it makes logical
> >> sense too -- if we can't determine something from the pointer reg, then we
> >> look at other info, such as the symbol_ref within a lo_sum.
> >>     
> >
> > Yes, it will accomplish the same thing for this particular case.
> >
> > Currently find_base_value and find_base_term differ in their handling
> > of LO_SUM.  My question is whether this fix should maintain the difference
> > in algorithms. 
> I suspect the differences are unintentional and  one could easily argue 
> that find_base_value's handling of LO_SUM is better since LO_SUM has 
> pretty well defined semantics.  One could also argue that both functions 
> shouldn't be so eager to return the results of the recursive call when 
> the recursive call returns NULL.
> 
> >  Also, RTL alias analysis is very fragile and your suggestion
> > may produce more accurate information in other situations, which could
> > expose other latent bugs or unexpected behavior.
> >   
> Part of the intent was to get the more accurate info in the presence of 
> registers marked with REG_POINTER.  While it could expose latent bugs, I 
> think it's the right thing to do -- though we might look for a change 
> with a smaller impact for the upcoming release since I think we're in 
> stage3.
> 
> jeff
> 

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

Hi,

The attached patch changes the handling of LO_SUM in "find_base_term" to
what "find_base_value" does.

No additional testsuite failures were noticed and SPEC2K numbers are
stable. Vortex 32-bit (that had degraded due to this problem) is back to
normal (up 3%) on PPC.

OK for mainline?

Thanks,
Luis

On Thu, 2008-11-06 at 16:16 -0700, Jeff Law wrote:
> David Edelsohn wrote:
> >
> >   
> >>> find_base_term() either could check for non-zero base before returning
> >>> in the lo_sum/plus/minus case statement or could handle lo_sum
> >>> explicitly, like find_base_value().
> >>>       
> >> I think my suggestion will accomplish the same thing -- and it makes logical
> >> sense too -- if we can't determine something from the pointer reg, then we
> >> look at other info, such as the symbol_ref within a lo_sum.
> >>     
> >
> > Yes, it will accomplish the same thing for this particular case.
> >
> > Currently find_base_value and find_base_term differ in their handling
> > of LO_SUM.  My question is whether this fix should maintain the difference
> > in algorithms. 
> I suspect the differences are unintentional and  one could easily argue 
> that find_base_value's handling of LO_SUM is better since LO_SUM has 
> pretty well defined semantics.  One could also argue that both functions 
> shouldn't be so eager to return the results of the recursive call when 
> the recursive call returns NULL.
> 
> >  Also, RTL alias analysis is very fragile and your suggestion
> > may produce more accurate information in other situations, which could
> > expose other latent bugs or unexpected behavior.
> >   
> Part of the intent was to get the more accurate info in the presence of 
> registers marked with REG_POINTER.  While it could expose latent bugs, I 
> think it's the right thing to do -- though we might look for a change 
> with a smaller impact for the upcoming release since I think we're in 
> stage3.
> 
> jeff
> 

2008-11-27  Luis Machado  <luisgpm@br.ibm.com>

	* alias.c (find_base_term): Synch LO_SUM handling with what
	find_base_value does.

Index: gcc/alias.c
===================================================================
--- gcc.orig/alias.c	2008-11-25 19:43:01.000000000 -0800
+++ gcc/alias.c	2008-11-27 04:54:09.000000000 -0800
@@ -1408,6 +1408,9 @@
 	return 0;
       /* Fall through.  */
     case LO_SUM:
+      /* The standard form is (lo_sum reg sym) so look only at the
+         second operand.  */
+      return find_base_term (XEXP (x, 1));
     case PLUS:
     case MINUS:
       {

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Richard Guenther]

On Thu, Nov 27, 2008 at 2:22 PM, Luis Machado
<luisgpm@linux.vnet.ibm.com> wrote:
> Hi,
>
> The attached patch changes the handling of LO_SUM in "find_base_term" to
> what "find_base_value" does.
>
> No additional testsuite failures were noticed and SPEC2K numbers are
> stable. Vortex 32-bit (that had degraded due to this problem) is back to
> normal (up 3%) on PPC.
>
> OK for mainline?

Ok.

Thanks,
Richard.

> Thanks,
> Luis
>
> On Thu, 2008-11-06 at 16:16 -0700, Jeff Law wrote:
>> David Edelsohn wrote:
>> >
>> >
>> >>> find_base_term() either could check for non-zero base before returning
>> >>> in the lo_sum/plus/minus case statement or could handle lo_sum
>> >>> explicitly, like find_base_value().
>> >>>
>> >> I think my suggestion will accomplish the same thing -- and it makes logical
>> >> sense too -- if we can't determine something from the pointer reg, then we
>> >> look at other info, such as the symbol_ref within a lo_sum.
>> >>
>> >
>> > Yes, it will accomplish the same thing for this particular case.
>> >
>> > Currently find_base_value and find_base_term differ in their handling
>> > of LO_SUM.  My question is whether this fix should maintain the difference
>> > in algorithms.
>> I suspect the differences are unintentional and  one could easily argue
>> that find_base_value's handling of LO_SUM is better since LO_SUM has
>> pretty well defined semantics.  One could also argue that both functions
>> shouldn't be so eager to return the results of the recursive call when
>> the recursive call returns NULL.
>>
>> >  Also, RTL alias analysis is very fragile and your suggestion
>> > may produce more accurate information in other situations, which could
>> > expose other latent bugs or unexpected behavior.
>> >
>> Part of the intent was to get the more accurate info in the presence of
>> registers marked with REG_POINTER.  While it could expose latent bugs, I
>> think it's the right thing to do -- though we might look for a change
>> with a smaller impact for the upcoming release since I think we're in
>> stage3.
>>
>> jeff
>>
>
> 2008-11-27  Luis Machado  <luisgpm@br.ibm.com>
>
>        * alias.c (find_base_term): Synch LO_SUM handling with what
>        find_base_value does.
>
> Index: gcc/alias.c
> ===================================================================
> --- gcc.orig/alias.c    2008-11-25 19:43:01.000000000 -0800
> +++ gcc/alias.c 2008-11-27 04:54:09.000000000 -0800
> @@ -1408,6 +1408,9 @@
>        return 0;
>       /* Fall through.  */
>     case LO_SUM:
> +      /* The standard form is (lo_sum reg sym) so look only at the
> +         second operand.  */
> +      return find_base_term (XEXP (x, 1));
>     case PLUS:
>     case MINUS:
>       {
>
>
>

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Luis Machado wrote:
> Hi,
>
> The attached patch changes the handling of LO_SUM in "find_base_term" to
> what "find_base_value" does.
>
> No additional testsuite failures were noticed and SPEC2K numbers are
> stable. Vortex 32-bit (that had degraded due to this problem) is back to
> normal (up 3%) on PPC.
>
> OK for mainline?
>   

It's fine.  Can you also create a new PR and link it to the 4.5 pending 
patches meta-PR?  For 4.5 I think we want to use a combination of the 
two approaches (short-circuit lo-sum and not return null so easily when 
the recursive calls return NULL).

jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

On Thu, 2008-11-27 at 12:16 -0700, Jeff Law wrote:
> Luis Machado wrote:
> > Hi,
> >
> > The attached patch changes the handling of LO_SUM in "find_base_term" to
> > what "find_base_value" does.
> >
> > No additional testsuite failures were noticed and SPEC2K numbers are
> > stable. Vortex 32-bit (that had degraded due to this problem) is back to
> > normal (up 3%) on PPC.
> >
> > OK for mainline?
> >   
> 
> It's fine.  Can you also create a new PR and link it to the 4.5 pending 
> patches meta-PR?  For 4.5 I think we want to use a combination of the 
> two approaches (short-circuit lo-sum and not return null so easily when 
> the recursive calls return NULL).

Sure, i'll add the PR.

Wouldn't it be best to combine both approaches now?

Thanks,
Luis

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Jeff Law]

Luis Machado wrote:
> On Thu, 2008-11-27 at 12:16 -0700, Jeff Law wrote:
>   
>> Luis Machado wrote:
>>     
>>> Hi,
>>>
>>> The attached patch changes the handling of LO_SUM in "find_base_term" to
>>> what "find_base_value" does.
>>>
>>> No additional testsuite failures were noticed and SPEC2K numbers are
>>> stable. Vortex 32-bit (that had degraded due to this problem) is back to
>>> normal (up 3%) on PPC.
>>>
>>> OK for mainline?
>>>   
>>>       
>> It's fine.  Can you also create a new PR and link it to the 4.5 pending 
>> patches meta-PR?  For 4.5 I think we want to use a combination of the 
>> two approaches (short-circuit lo-sum and not return null so easily when 
>> the recursive calls return NULL).
>>     
>
> Sure, i'll add the PR.
>
> Wouldn't it be best to combine both approaches now?
>   
In the interest of minimizing changes which might destabilize the tree, 
I think it's best to take the minimalist approach for 4.4 bits.  Once 
4.4 branches we can install the more aggressive approach.

jeff

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

Hi folks,

On Thu, 2008-11-27 at 14:08 -0700, Jeff Law wrote:
> Luis Machado wrote:
> > On Thu, 2008-11-27 at 12:16 -0700, Jeff Law wrote:
> >   
> >> Luis Machado wrote:
> >>     
> >>> Hi,
> >>>
> >>> The attached patch changes the handling of LO_SUM in "find_base_term" to
> >>> what "find_base_value" does.
> >>>
> >>> No additional testsuite failures were noticed and SPEC2K numbers are
> >>> stable. Vortex 32-bit (that had degraded due to this problem) is back to
> >>> normal (up 3%) on PPC.
> >>>
> >>> OK for mainline?
> >>>   
> >>>       
> >> It's fine.  Can you also create a new PR and link it to the 4.5 pending 
> >> patches meta-PR?  For 4.5 I think we want to use a combination of the 
> >> two approaches (short-circuit lo-sum and not return null so easily when 
> >> the recursive calls return NULL).
> >>     
> >
> > Sure, i'll add the PR.
> >
> > Wouldn't it be best to combine both approaches now?
> >   
> In the interest of minimizing changes which might destabilize the tree, 
> I think it's best to take the minimalist approach for 4.4 bits.  Once 
> 4.4 branches we can install the more aggressive approach.
> 
> jeff

As 4.4 has already branched, follows the additional part of the RTL
alias fix that should go in for 4.5, checking for NULL before returning
the base term.

The testsuite numbers are good. Regtested on powerpc-linux.

Ok?

Regards,
Luis


2009-05-26  Luis Machado  <luisgpm@br.ibm.com>

	* alias.c (find_base_term): Check for NULL term before returning.

Index: gcc/alias.c
===================================================================
--- gcc.orig/alias.c	2009-05-25 20:30:33.000000000 -0700
+++ gcc/alias.c	2009-05-25 20:30:50.000000000 -0700
@@ -1511,10 +1511,18 @@
 	/* If either operand is known to be a pointer, then use it
 	   to determine the base term.  */
 	if (REG_P (tmp1) && REG_POINTER (tmp1))
-	  return find_base_term (tmp1);
+	  {
+	    rtx base = find_base_term (tmp1);
+	    if (base)
+	      return base;
+	  }
 
 	if (REG_P (tmp2) && REG_POINTER (tmp2))
-	  return find_base_term (tmp2);
+	  {
+	    rtx base = find_base_term (tmp2);
+	    if (base)
+	      return base;
+	  }
 
 	/* Neither operand was known to be a pointer.  Go ahead and find the
 	   base term for both operands.  */

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Ian Lance Taylor]

Luis Machado <luisgpm@linux.vnet.ibm.com> writes:

> 2009-05-26  Luis Machado  <luisgpm@br.ibm.com>
>
> 	* alias.c (find_base_term): Check for NULL term before returning.

This is OK.

Thanks.

Ian

Re: Patch to fix gcc.c-torture/compile/20010102-1.c on IA64 HP-UX

[Luis Machado]

Thanks Ian.

I've checked this in now.

Regards,
Luis

On Sun, 2009-05-31 at 20:55 -0700, Ian Lance Taylor wrote:
> Luis Machado <luisgpm@linux.vnet.ibm.com> writes:
> 
> > 2009-05-26  Luis Machado  <luisgpm@br.ibm.com>
> >
> > 	* alias.c (find_base_term): Check for NULL term before returning.
> 
> This is OK.
> 
> Thanks.
> 
> Ian