Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-13 Thread Jeff Law

On 02/12/15 05:40, Alex Velenko wrote:

On 09/02/15 23:32, Jeff Law wrote:

On 02/03/15 20:03, Bin.Cheng wrote:

I looked into the test and can confirm the previous compilation is
correct.
The cover letter of this patch said IRA mis-handled REQ_EQUIV before,
but in this case it is REG_EQUAL that is lost.  The full dump (without
this patch) after IRA is like:

Right, but a REG_EQUIV is generated based on the incoming REG_EQUAL
notes in the insn stream.  Basically update_equiv_regs will scan insn
stream and some REG_EQUAL notes will be promoted to REG_EQUIV notes.


Hi Jeff,
Do I understand you correctly, that REG_EQUAL notes should not be
generated in IRA pass, because some of them may get promoted to
REG_EQUIV? Or is there any other reason register r110 should not get
REG_EQUAL note? Previously, register r110 was getting REG_EQUAL, not
REG_EQUIV note.
The reason r110 can not get a REG_EQUIV note is because there are 
multiple insns that set r110 to different values.


The equivalency created by a REG_EQUIV note has to be valid at every 
point within the function.  Thus for any pseudo that holds > 1 distinct 
value within a function there can be no valid REG_EQUIV notes on the 
assignments to that pseudo.




jeff



Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-12 Thread Alex Velenko

On 09/02/15 23:32, Jeff Law wrote:

On 02/03/15 20:03, Bin.Cheng wrote:

I looked into the test and can confirm the previous compilation is correct.
The cover letter of this patch said IRA mis-handled REQ_EQUIV before,
but in this case it is REG_EQUAL that is lost.  The full dump (without
this patch) after IRA is like:

Right, but a REG_EQUIV is generated based on the incoming REG_EQUAL
notes in the insn stream.  Basically update_equiv_regs will scan insn
stream and some REG_EQUAL notes will be promoted to REG_EQUIV notes.


Hi Jeff,
Do I understand you correctly, that REG_EQUAL notes should not be
generated in IRA pass, because some of them may get promoted to
REG_EQUIV? Or is there any other reason register r110 should not get
REG_EQUAL note? Previously, register r110 was getting REG_EQUAL, not
REG_EQUIV note.
Kind regards,
Alex



The REG_EQUIV is a function-wide equivalence, meaning that one could
substitute the REG_EQUIV note for in any uses of the destination
register and still have a valid representation of the program.

REG_EQUAL's validity is limited to the point after the insn in which it
appears and before the next insn.



Before r216169 (with REG_EQUAL in insn9), jumps from basic block 6/7/8
-> 9 can be merged because r110 equals to -1 afterwards.  But with the
patch, the equal information of r110==-1 in basic block 8 is lost.  As
a result, jump from 8->9 can't be merged and two additional
instructions are generated.

  >

I suppose the REG_EQUAL note is correct in insn9?  According to
GCCint, it only means r110 set by insn9 will be equal to the value at
run time at the end of this insn but not necessarily elsewhere in the
function.

If you previously got a REG_EQUIV note on any of those insns it was
wrong and this is the precise kind of situation that the change was
trying to fix.

R110 can have the value -1 (BB6, BB7, BB8) or 0 (BB5).  Thus there is no
single value across the entire function that one can validly use for r110.

I think you could mark this as a missed optimization, but not a
regresssion since the desired output was relying on a bug in the compiler.

If I were to start looking at this, my first thought would be to look at
why we have multiple sets of r110, particularly if there are lifetimes
that are disjoint.




I also found another problem (or mis-leading?) with the document:
"Thus, compiler passes prior to register allocation need only check
for REG_EQUAL notes and passes subsequent to register allocation need
only check for REG_EQUIV notes".  This seems not true now as in this
example, passes after register allocation do take advantage of
REG_EQUAL in optimization and we can't achieve that by using
REG_EQUIV.

I think that's a long standing (and incorrect) generalization.  IIRC we
can get a REG_EQUIV note earlier for certain argument setup situations.
   And I think it's been the case for a long time that a pass after
reload could try to exploit REG_EQUAL notes.

jeff





Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-10 Thread Jeff Law

On 02/10/15 03:51, Ajit Kumar Agarwal wrote:

R110 can have the value -1 (BB6, BB7, BB8) or 0 (BB5).  Thus there is no single 
value across the entire function that one can validly use for r110.


Does the value of R110 should not change across all the callee path from the 
given caller functions.  If the value is aliased, then how the call side 
affects should make
sure the value remains same across all the callee chain path from  the given 
caller function. I am curious to know how the value remain constant throughout 
the function
  is identified in case of aliasing and the interprocedural case.

Pseudo registers are allowed to have aliases.


Jeff



RE: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-10 Thread Ajit Kumar Agarwal


-Original Message-
From: gcc-patches-ow...@gcc.gnu.org [mailto:gcc-patches-ow...@gcc.gnu.org] On 
Behalf Of Jeff Law
Sent: Tuesday, February 10, 2015 5:03 AM
To: Bin.Cheng
Cc: Alex Velenko; Felix Yang; Yangfei (Felix); Marcus Shawcroft; GCC Patches; 
vmaka...@redhat.com
Subject: Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for 
pseudo with more than one definition

On 02/03/15 20:03, Bin.Cheng wrote:
> I looked into the test and can confirm the previous compilation is correct.
> The cover letter of this patch said IRA mis-handled REQ_EQUIV before, 
> but in this case it is REG_EQUAL that is lost.  The full dump (without 
> this patch) after IRA is like:
Right, but a REG_EQUIV is generated based on the incoming REG_EQUAL notes in 
the insn stream.  Basically update_equiv_regs will scan insn stream and some 
REG_EQUAL notes will be promoted to REG_EQUIV notes.

The REG_EQUIV is a function-wide equivalence, meaning that one could substitute 
the REG_EQUIV note for in any uses of the destination register and still have a 
valid representation of the program.

REG_EQUAL's validity is limited to the point after the insn in which it appears 
and before the next insn.

>
> Before r216169 (with REG_EQUAL in insn9), jumps from basic block 6/7/8
> -> 9 can be merged because r110 equals to -1 afterwards.  But with the
> patch, the equal information of r110==-1 in basic block 8 is lost.  As 
> a result, jump from 8->9 can't be merged and two additional 
> instructions are generated.
 >
> I suppose the REG_EQUAL note is correct in insn9?  According to 
> GCCint, it only means r110 set by insn9 will be equal to the value at 
> run time at the end of this insn but not necessarily elsewhere in the 
> function.
>>If you previously got a REG_EQUIV note on any of those insns it was wrong and 
>>this is the precise kind of situation that the change was trying to fix.

>>R110 can have the value -1 (BB6, BB7, BB8) or 0 (BB5).  Thus there is no 
>>single value across the entire function that one can validly use for r110.

Does the value of R110 should not change across all the callee path from the 
given caller functions.  If the value is aliased, then how the call side 
affects should make
sure the value remains same across all the callee chain path from  the given 
caller function. I am curious to know how the value remain constant throughout 
the function
 is identified in case of aliasing and the interprocedural case.

>>I think you could mark this as a missed optimization, but not a regresssion 
>>since the desired output was relying on a bug in the compiler.
>>
>>If I were to start looking at this, my first thought would be to look at why 
>>we have multiple sets of r110, particularly if there are lifetimes that are 
>>disjoint.


>
> I also found another problem (or mis-leading?) with the document:
> "Thus, compiler passes prior to register allocation need only check 
> for REG_EQUAL notes and passes subsequent to register allocation need 
> only check for REG_EQUIV notes".  This seems not true now as in this 
> example, passes after register allocation do take advantage of 
> REG_EQUAL in optimization and we can't achieve that by using 
> REG_EQUIV.
>>I think that's a long standing (and incorrect) generalization.  IIRC we can 
>>get a REG_EQUIV note earlier for certain argument setup situations. 
  >>And I think it's been the case for a long time that a pass after reload 
could try to exploit REG_EQUAL notes.

Thanks & Regards
Ajit
jeff


Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-09 Thread Jeff Law

On 02/03/15 20:03, Bin.Cheng wrote:

I looked into the test and can confirm the previous compilation is correct.
The cover letter of this patch said IRA mis-handled REQ_EQUIV before,
but in this case it is REG_EQUAL that is lost.  The full dump (without
this patch) after IRA is like:
Right, but a REG_EQUIV is generated based on the incoming REG_EQUAL 
notes in the insn stream.  Basically update_equiv_regs will scan insn 
stream and some REG_EQUAL notes will be promoted to REG_EQUIV notes.


The REG_EQUIV is a function-wide equivalence, meaning that one could 
substitute the REG_EQUIV note for in any uses of the destination 
register and still have a valid representation of the program.


REG_EQUAL's validity is limited to the point after the insn in which it 
appears and before the next insn.




Before r216169 (with REG_EQUAL in insn9), jumps from basic block 6/7/8
-> 9 can be merged because r110 equals to -1 afterwards.  But with the
patch, the equal information of r110==-1 in basic block 8 is lost.  As
a result, jump from 8->9 can't be merged and two additional
instructions are generated.

>

I suppose the REG_EQUAL note is correct in insn9?  According to
GCCint, it only means r110 set by insn9 will be equal to the value at
run time at the end of this insn but not necessarily elsewhere in the
function.
If you previously got a REG_EQUIV note on any of those insns it was 
wrong and this is the precise kind of situation that the change was 
trying to fix.


R110 can have the value -1 (BB6, BB7, BB8) or 0 (BB5).  Thus there is no 
single value across the entire function that one can validly use for r110.


I think you could mark this as a missed optimization, but not a 
regresssion since the desired output was relying on a bug in the compiler.


If I were to start looking at this, my first thought would be to look at 
why we have multiple sets of r110, particularly if there are lifetimes 
that are disjoint.





I also found another problem (or mis-leading?) with the document:
"Thus, compiler passes prior to register allocation need only check
for REG_EQUAL notes and passes subsequent to register allocation need
only check for REG_EQUIV notes".  This seems not true now as in this
example, passes after register allocation do take advantage of
REG_EQUAL in optimization and we can't achieve that by using
REG_EQUIV.
I think that's a long standing (and incorrect) generalization.  IIRC we 
can get a REG_EQUIV note earlier for certain argument setup situations. 
 And I think it's been the case for a long time that a pass after 
reload could try to exploit REG_EQUAL notes.


jeff


Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-03 Thread Bin.Cheng
On Wed, Feb 4, 2015 at 12:28 AM, Jeff Law  wrote:
> On 02/03/15 01:29, Bin.Cheng wrote:
>>
>>
>> Hmm, if I understand correctly, it's a code size regression, so I
>> don't think it's appropriate to adapt the test case.  Either the patch
>> or something else in GCC is doing wrong, right?
>>
>> Hi Alex, could you please file a PR with full dump information for
>> tracking?
>
> But if the code size regression is due to the older compiler incorrectly
> handling the promotion of REG_EQUAL to REG_EQUIV notes, then the test
> absolutely does need updating as the codesize was dependent on incorrect
> behaviour in the compiler.

Hi Jeff,

I looked into the test and can confirm the previous compilation is correct.
The cover letter of this patch said IRA mis-handled REQ_EQUIV before,
but in this case it is REG_EQUAL that is lost.  The full dump (without
this patch) after IRA is like:

   10: NOTE_INSN_BASIC_BLOCK 2
2: r116:SI=r0:SI
3: r117:SI=r1:SI
  REG_DEAD r1:SI
4: r118:SI=r2:SI
  REG_DEAD r2:SI
5: NOTE_INSN_FUNCTION_BEG
   12: r2:SI=0x1
   13: r1:SI=0
   15: r0:SI=call [`lseek'] argc:0
  REG_DEAD r2:SI
  REG_DEAD r1:SI
  REG_CALL_DECL `lseek'
   16: r111:SI=r0:SI
  REG_DEAD r0:SI
   17: r2:SI=0x2
   18: r1:SI=0
   19: r0:SI=r116:SI
  REG_DEAD r116:SI
   20: r0:SI=call [`lseek'] argc:0
  REG_DEAD r2:SI
  REG_DEAD r1:SI
  REG_CALL_DECL `lseek'
   21: r112:SI=r0:SI
  REG_DEAD r0:SI
   22: cc:CC=cmp(r111:SI,0x)
   23: pc={(cc:CC==0)?L46:pc}
  REG_DEAD cc:CC
  REG_BR_PROB 159
   24: NOTE_INSN_BASIC_BLOCK 3
   25: cc:CC=cmp(r112:SI,0x)
   26: pc={(cc:CC==0)?L50:pc}
  REG_DEAD cc:CC
  REG_BR_PROB 159
   27: NOTE_INSN_BASIC_BLOCK 4
   28: NOTE_INSN_DELETED
   29: {cc:CC_NOOV=cmp(r112:SI-r111:SI,0);r114:SI=r112:SI-r111:SI;}
  REG_DEAD r112:SI
   30: pc={(cc:CC_NOOV==0)?L54:pc}
  REG_DEAD cc:CC_NOOV
  REG_BR_PROB 400
   31: NOTE_INSN_BASIC_BLOCK 5
   32: [r117:SI]=r111:SI
  REG_DEAD r117:SI
  REG_DEAD r111:SI
   33: [r118:SI]=r114:SI
  REG_DEAD r118:SI
  REG_DEAD r114:SI
7: r110:SI=0
  REG_EQUAL 0
   76: pc=L34
   77: barrier
   46: L46:
   45: NOTE_INSN_BASIC_BLOCK 6
8: r110:SI=r111:SI
  REG_DEAD r111:SI
  REG_EQUAL 0x
   78: pc=L34
   79: barrier
   50: L50:
   49: NOTE_INSN_BASIC_BLOCK 7
6: r110:SI=r112:SI
  REG_DEAD r112:SI
  REG_EQUAL 0x
   80: pc=L34
   81: barrier
   54: L54:
   53: NOTE_INSN_BASIC_BLOCK 8
9: r110:SI=0x
  REG_EQUAL 0x
   34: L34:
   35: NOTE_INSN_BASIC_BLOCK 9
   40: r0:SI=r110:SI
  REG_DEAD r110:SI
   41: use r0:SI

Before r216169 (with REG_EQUAL in insn9), jumps from basic block 6/7/8
-> 9 can be merged because r110 equals to -1 afterwards.  But with the
patch, the equal information of r110==-1 in basic block 8 is lost.  As
a result, jump from 8->9 can't be merged and two additional
instructions are generated.

I suppose the REG_EQUAL note is correct in insn9?  According to
GCCint, it only means r110 set by insn9 will be equal to the value at
run time at the end of this insn but not necessarily elsewhere in the
function.

I also found another problem (or mis-leading?) with the document:
"Thus, compiler passes prior to register allocation need only check
for REG_EQUAL notes and passes subsequent to register allocation need
only check for REG_EQUIV notes".  This seems not true now as in this
example, passes after register allocation do take advantage of
REG_EQUAL in optimization and we can't achieve that by using
REG_EQUIV.

Thanks,
bin

>
> jeff


Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-03 Thread Jeff Law

On 02/03/15 01:29, Bin.Cheng wrote:


Hmm, if I understand correctly, it's a code size regression, so I
don't think it's appropriate to adapt the test case.  Either the patch
or something else in GCC is doing wrong, right?

Hi Alex, could you please file a PR with full dump information for tracking?
But if the code size regression is due to the older compiler incorrectly 
handling the promotion of REG_EQUAL to REG_EQUIV notes, then the test 
absolutely does need updating as the codesize was dependent on incorrect 
behaviour in the compiler.


jeff


Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-03 Thread Alex Velenko

On 03/02/15 08:29, Bin.Cheng wrote:

On Tue, Feb 3, 2015 at 3:24 PM, Jeff Law  wrote:

On 02/02/15 08:59, Alex Velenko wrote:


On 11/10/14 13:44, Felix Yang wrote:


Hello Jeff,

  I see that you have improved the RTL typesafety issue for ira.c,
so I rebased this patch
  on the latest trunk and change to use the new list walking
interface.
  Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
  OK for trunk?


Hi Felix,
I believe your patch causes a regression for arm-none-eabi.
FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2

This happens because your patch stops reuse of code for
" return -1;" statements in pr43920-2.c.

As far as I investigated, your patch prevents adding "(expr_list (-1)
(nil)" in ira pass, which prevents jump2 optimization from happening.

So before, in ira pass I could see:
"(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
  (const_int -1 [0x]))
/work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
613
{*thumb2_movsi_vfp}
   (expr_list:REG_EQUAL (const_int -1 [0x])
  (nil)))"
But with your patch I get
"(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
  (const_int -1 [0x]))
/work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
615 {*thumb2_movsi_vfp}
   (nil))"

This causes a code generation regression and needs to be fixed.
Kind regards,


We'd need to see the full dumps.  In particular is reg110 set anywhere else?
If so then the change is doing precisely what it should be doing and the
test needs to be updated to handle the different code we generate.


Hmm, if I understand correctly, it's a code size regression, so I
don't think it's appropriate to adapt the test case.  Either the patch
or something else in GCC is doing wrong, right?

Hi Alex, could you please file a PR with full dump information for tracking?

Thanks,
bin

Hi Bin,
Created bugzilla ticket, as requested:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64916
This test already existed in the testsuite, it is not new.
Kind regards,
Alex



Jeff






Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-03 Thread Bin.Cheng
On Tue, Feb 3, 2015 at 3:24 PM, Jeff Law  wrote:
> On 02/02/15 08:59, Alex Velenko wrote:
>>
>> On 11/10/14 13:44, Felix Yang wrote:
>>>
>>> Hello Jeff,
>>>
>>>  I see that you have improved the RTL typesafety issue for ira.c,
>>> so I rebased this patch
>>>  on the latest trunk and change to use the new list walking
>>> interface.
>>>  Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
>>>  OK for trunk?
>>
>> Hi Felix,
>> I believe your patch causes a regression for arm-none-eabi.
>> FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
>> FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2
>>
>> This happens because your patch stops reuse of code for
>> " return -1;" statements in pr43920-2.c.
>>
>> As far as I investigated, your patch prevents adding "(expr_list (-1)
>> (nil)" in ira pass, which prevents jump2 optimization from happening.
>>
>> So before, in ira pass I could see:
>> "(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
>>  (const_int -1 [0x]))
>> /work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
>> 613
>> {*thumb2_movsi_vfp}
>>   (expr_list:REG_EQUAL (const_int -1 [0x])
>>  (nil)))"
>> But with your patch I get
>> "(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
>>  (const_int -1 [0x]))
>> /work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
>> 615 {*thumb2_movsi_vfp}
>>   (nil))"
>>
>> This causes a code generation regression and needs to be fixed.
>> Kind regards,
>
> We'd need to see the full dumps.  In particular is reg110 set anywhere else?
> If so then the change is doing precisely what it should be doing and the
> test needs to be updated to handle the different code we generate.

Hmm, if I understand correctly, it's a code size regression, so I
don't think it's appropriate to adapt the test case.  Either the patch
or something else in GCC is doing wrong, right?

Hi Alex, could you please file a PR with full dump information for tracking?

Thanks,
bin
>
> Jeff


Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-02 Thread Jeff Law

On 02/02/15 08:59, Alex Velenko wrote:

On 11/10/14 13:44, Felix Yang wrote:

Hello Jeff,

 I see that you have improved the RTL typesafety issue for ira.c,
so I rebased this patch
 on the latest trunk and change to use the new list walking
interface.
 Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
 OK for trunk?

Hi Felix,
I believe your patch causes a regression for arm-none-eabi.
FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2

This happens because your patch stops reuse of code for
" return -1;" statements in pr43920-2.c.

As far as I investigated, your patch prevents adding "(expr_list (-1)
(nil)" in ira pass, which prevents jump2 optimization from happening.

So before, in ira pass I could see:
"(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
 (const_int -1 [0x]))
/work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20 613
{*thumb2_movsi_vfp}
  (expr_list:REG_EQUAL (const_int -1 [0x])
 (nil)))"
But with your patch I get
"(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
 (const_int -1 [0x]))
/work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20
615 {*thumb2_movsi_vfp}
  (nil))"

This causes a code generation regression and needs to be fixed.
Kind regards,
We'd need to see the full dumps.  In particular is reg110 set anywhere 
else?  If so then the change is doing precisely what it should be doing 
and the test needs to be updated to handle the different code we generate.


Jeff


Re: Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2015-02-02 Thread Alex Velenko

On 11/10/14 13:44, Felix Yang wrote:

Hello Jeff,

 I see that you have improved the RTL typesafety issue for ira.c,
so I rebased this patch
 on the latest trunk and change to use the new list walking interface.
 Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
 OK for trunk?

Hi Felix,
I believe your patch causes a regression for arm-none-eabi.
FAIL: gcc.target/arm/pr43920-2.c object-size text <= 54
FAIL: gcc.target/arm/pr43920-2.c scan-assembler-times pop 2

This happens because your patch stops reuse of code for
" return -1;" statements in pr43920-2.c.

As far as I investigated, your patch prevents adding "(expr_list (-1) 
(nil)" in ira pass, which prevents jump2 optimization from happening.


So before, in ira pass I could see:
"(insn 9 53 34 8 (set (reg:SI 110 [ D.4934 ])
(const_int -1 [0x])) 
/work/fsf-trunk-ref-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20 613 
{*thumb2_movsi_vfp}

 (expr_list:REG_EQUAL (const_int -1 [0x])
(nil)))"
But with your patch I get
"(insn 9 53 34 8 (set (reg:SI 110 [ D.5322 ])
(const_int -1 [0x])) 
/work/fsf-trunk-2/src/gcc/gcc/testsuite/gcc.target/arm/pr43920-2.c:20 
615 {*thumb2_movsi_vfp}

 (nil))"

This causes a code generation regression and needs to be fixed.
Kind regards,
Alex



Index: gcc/ChangeLog
===
--- gcc/ChangeLog(revision 216116)
+++ gcc/ChangeLog(working copy)
@@ -1,3 +1,14 @@
+2014-10-11  Felix Yang  
+Jeff Law  
+
+* ira.c (struct equivalence): Change member "is_arg_equivalence"
and "replace"
+into boolean bitfields; turn member "loop_depth" into a short
integer; add new
+member "no_equiv" and "reserved".
+(no_equiv): Set no_equiv of struct equivalence if register is marked
+as having no known equivalence.
+(update_equiv_regs): Check all definitions for a multiple-set
+register to make sure that the RHS have the same value.
+
  2014-10-11  Martin Liska  

  PR/63376
Index: gcc/ira.c
===
--- gcc/ira.c(revision 216116)
+++ gcc/ira.c(working copy)
@@ -2902,12 +2902,14 @@ struct equivalence

/* Loop depth is used to recognize equivalences which appear
   to be present within the same loop (or in an inner loop).  */
-  int loop_depth;
+  short loop_depth;
/* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
/* Set when an attempt should be made to replace a register
   with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
  };

  /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3255,6 +3257,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
if (!REG_P (reg))
  return;
regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
list = reg_equiv[regno].init_insns;
if (list && list->insn () == NULL)
  return;
@@ -3381,7 +3384,7 @@ update_equiv_regs (void)

/* If this insn contains more (or less) than a single SET,
   only mark all destinations as having no known equivalence.  */
-  if (set == 0)
+  if (set == NULL_RTX)
  {
note_stores (PATTERN (insn), no_equiv, NULL);
continue;
@@ -3476,16 +3479,49 @@ update_equiv_regs (void)
if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
  note = NULL_RTX;

-  if (DF_REG_DEF_COUNT (regno) != 1
-  && (! note
+  if (DF_REG_DEF_COUNT (regno) != 1)
+{
+  bool equal_p = true;
+  rtx_insn_list *list;
+
+  /* If we have already processed this pseudo and determined it
+ can not have an equivalence, then honor that decision.  */
+  if (reg_equiv[regno].no_equiv)
+continue;
+
+  if (! note
|| rtx_varies_p (XEXP (note, 0), 0)
|| (reg_equiv[regno].replacement
&& ! rtx_equal_p (XEXP (note, 0),
-reg_equiv[regno].replacement
-{
-  no_equiv (dest, set, NULL);
-  continue;
+reg_equiv[regno].replacement)))
+{
+  no_equiv (dest, set, NULL);
+  continue;
+}
+
+  list = reg_equiv[regno].init_insns;
+  for (; list; list = list->next ())
+{
+  rtx note_tmp;
+  rtx_insn *insn_tmp;
+
+  insn_tmp = list->insn ();
+  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+  gcc_assert (note_tmp);
+  if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+{
+  equal_p = false;
+  break;
+}
+}
+
+  if (! equal_p)
+{
+  no_equiv (dest, set, NULL);
+  c

Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-10-13 Thread Jeff Law

On 10/11/14 06:44, Felix Yang wrote:

Hello Jeff,

 I see that you have improved the RTL typesafety issue for ira.c,
so I rebased this patch
 on the latest trunk and change to use the new list walking interface.
 Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
 OK for trunk?

OK for the trunk.

Thanks for your patience.

jeff



Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-10-11 Thread Felix Yang
Hello Jeff,

I see that you have improved the RTL typesafety issue for ira.c,
so I rebased this patch
on the latest trunk and change to use the new list walking interface.
Bootstrapped on x86_64-SUSE-Linux and make check regression tested.
OK for trunk?

Index: gcc/ChangeLog
===
--- gcc/ChangeLog(revision 216116)
+++ gcc/ChangeLog(working copy)
@@ -1,3 +1,14 @@
+2014-10-11  Felix Yang  
+Jeff Law  
+
+* ira.c (struct equivalence): Change member "is_arg_equivalence"
and "replace"
+into boolean bitfields; turn member "loop_depth" into a short
integer; add new
+member "no_equiv" and "reserved".
+(no_equiv): Set no_equiv of struct equivalence if register is marked
+as having no known equivalence.
+(update_equiv_regs): Check all definitions for a multiple-set
+register to make sure that the RHS have the same value.
+
 2014-10-11  Martin Liska  

 PR/63376
Index: gcc/ira.c
===
--- gcc/ira.c(revision 216116)
+++ gcc/ira.c(working copy)
@@ -2902,12 +2902,14 @@ struct equivalence

   /* Loop depth is used to recognize equivalences which appear
  to be present within the same loop (or in an inner loop).  */
-  int loop_depth;
+  short loop_depth;
   /* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
   /* Set when an attempt should be made to replace a register
  with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
 };

 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3255,6 +3257,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
 return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list && list->insn () == NULL)
 return;
@@ -3381,7 +3384,7 @@ update_equiv_regs (void)

   /* If this insn contains more (or less) than a single SET,
  only mark all destinations as having no known equivalence.  */
-  if (set == 0)
+  if (set == NULL_RTX)
 {
   note_stores (PATTERN (insn), no_equiv, NULL);
   continue;
@@ -3476,16 +3479,49 @@ update_equiv_regs (void)
   if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 note = NULL_RTX;

-  if (DF_REG_DEF_COUNT (regno) != 1
-  && (! note
+  if (DF_REG_DEF_COUNT (regno) != 1)
+{
+  bool equal_p = true;
+  rtx_insn_list *list;
+
+  /* If we have already processed this pseudo and determined it
+ can not have an equivalence, then honor that decision.  */
+  if (reg_equiv[regno].no_equiv)
+continue;
+
+  if (! note
   || rtx_varies_p (XEXP (note, 0), 0)
   || (reg_equiv[regno].replacement
   && ! rtx_equal_p (XEXP (note, 0),
-reg_equiv[regno].replacement
-{
-  no_equiv (dest, set, NULL);
-  continue;
+reg_equiv[regno].replacement)))
+{
+  no_equiv (dest, set, NULL);
+  continue;
+}
+
+  list = reg_equiv[regno].init_insns;
+  for (; list; list = list->next ())
+{
+  rtx note_tmp;
+  rtx_insn *insn_tmp;
+
+  insn_tmp = list->insn ();
+  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+  gcc_assert (note_tmp);
+  if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+{
+  equal_p = false;
+  break;
+}
+}
+
+  if (! equal_p)
+{
+  no_equiv (dest, set, NULL);
+  continue;
+}
 }
+
   /* Record this insn as initializing this register.  */
   reg_equiv[regno].init_insns
 = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3514,10 +3550,9 @@ update_equiv_regs (void)
  a register used only in one basic block from a MEM.  If so, and the
  MEM remains unchanged for the life of the register, add a REG_EQUIV
  note.  */
-
   note = find_reg_note (insn, REG_EQUIV, NULL_RTX);

-  if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+  if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
   && MEM_P (SET_SRC (set))
   && validate_equiv_mem (insn, dest, SET_SRC (set)))
 note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)));
@@ -3547,7 +3582,7 @@ update_equiv_regs (void)

   reg_equiv[regno].replacement = x;
   reg_equiv[regno].src_p = &SET_SRC (set);
-  reg_equiv[regno].loop_depth = loop_depth;
+  reg_equiv[regno].loop_depth = (short) loop_depth;

   /* Don't mess with things live dur

Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-29 Thread Jeff Law

On 09/27/14 08:48, Felix Yang wrote:

Thanks for the explaination.
I have changed the loop_depth into a short interger hoping that we can
save some memory :-)

Thanks.


Attached please find the updated patch. Bootstrapped and reg-tested on
x86_64-suse-linux.
Please do a final revew once the assignment is ready.

As for the new list walking interface, I choose the function
"no_equiv" and tried the "checked cast" way.
The bad news is that GCC failed to bootstrap with the following change:

Index: ira.c
===
--- ira.c (revision 215536)
+++ ira.c (working copy)
@@ -3242,12 +3242,12 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
void *data ATTRIBUTE_UNUSED)
  {
int regno;
-  rtx list;
+  rtx_insn_list *list;

if (!REG_P (reg))
  return;
regno = REGNO (reg);
-  list = reg_equiv[regno].init_insns;
+  list = as_a  (reg_equiv[regno].init_insns);
if (list == const0_rtx)
  return;
reg_equiv[regno].init_insns = const0_rtx;
@@ -3258,9 +3258,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
  return;
ira_reg_equiv[regno].defined_p = false;
ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = list->next ())
  {
-  rtx insn = XEXP (list, 0);
+  rtx_insn *insn = list->insn ();
remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
  }
  }
Yea.  I'm going to post a patch shortly to go ahead with this 
conversion.  There's a couple issues that come into play.


First const0_rtx is not an INSN, so we *really* don't want it in the 
INSN field of an INSN_LIST.  That's probably the ICE you're seeing.


const0_rtx is being used to mark pseudos which we've already determined 
can't have a valid equivalence.  So we just need a different marker. 
That different marker must be embeddable in an INSN_LIST node.   The 
easiest is just a NULL insn ;-)


The other tests for the const0_rtx marker in ira.c need relatively 
trivial updating.  And in the end we don't need the checked cast at all ;-)




Jeff


Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-27 Thread Felix Yang
Thanks for the explaination.
I have changed the loop_depth into a short interger hoping that we can
save some memory :-)
Attached please find the updated patch. Bootstrapped and reg-tested on
x86_64-suse-linux.
Please do a final revew once the assignment is ready.

As for the new list walking interface, I choose the function
"no_equiv" and tried the "checked cast" way.
The bad news is that GCC failed to bootstrap with the following change:

Index: ira.c
===
--- ira.c (revision 215536)
+++ ira.c (working copy)
@@ -3242,12 +3242,12 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   void *data ATTRIBUTE_UNUSED)
 {
   int regno;
-  rtx list;
+  rtx_insn_list *list;

   if (!REG_P (reg))
 return;
   regno = REGNO (reg);
-  list = reg_equiv[regno].init_insns;
+  list = as_a  (reg_equiv[regno].init_insns);
   if (list == const0_rtx)
 return;
   reg_equiv[regno].init_insns = const0_rtx;
@@ -3258,9 +3258,9 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
 return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = list->next ())
 {
-  rtx insn = XEXP (list, 0);
+  rtx_insn *insn = list->insn ();
   remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
 }
 }

Error message:
 ...
checking for suffix of object files... configure: error: in
`/home/yangfei/gcc-devel/build/x86_64-unknown-linux-gnu/libgcc':
configure: error: cannot compute suffix of object files: cannot compile
See `config.log' for more details.
make[2]: *** [configure-stage1-target-libgcc] Error 1
make[2]: Leaving directory `/home/yangfei/gcc-devel/build'
make[1]: *** [stage1-bubble] Error 2
make[1]: Leaving directory `/home/yangfei/gcc-devel/build'
make: *** [all] Error 2

I think the code change is OK. Anything I missed?

Cheers,
Felix


On Sat, Sep 27, 2014 at 5:03 AM, Jeff Law  wrote:
> On 09/26/14 07:57, Felix Yang wrote:
>>
>> Hi Jeff,
>>
>>  Thanks for the suggestions. I updated the patch accordingly.
>>
>>  1. Both my employer(Huawei) and I have signed the copyright
>> assignments with FSF.
>>  These assignments are already sent via post two days ago and
>> hopefully should reach FSF in one week.
>>  Maybe it's OK to commit this patch now?
>
> Not really.  It needs to be accepted by the FSF before we can include the
> work.
>
>>
>>   2. I am not turning member loop_depth of struct equivalence into
>> short integer as GCC API such as bb_loop_depth
>>   returns a loop's depth as a 32-bit interger.
>
> There's already other places that assume loops don't nest that deep. Please
> go ahead and change it.  And no need to explicitly mark the unused bits.
> That's just a maintenance nightmare in the long term anyway :-)
>
>
>>
>>   3. I find it's kind of difficult to use the new type and
>> interfaces for list walking the init_insns list for this patch.
>>  The type of init_insns list is rtx, not rtl_insn_list *. Seems
>> we need to change a lot in order to use the new interface.
>>  Not clear about the reason why it is not adjusted when we are
>> transferring to the new interface.
>>  Anyway, I think it's better to have another patch fix that issue.
>> OK?
>
> The right way to go is to add a checked cast when we have some code that is
> using the old interface and other code using the new interface.  It's
> actually a pretty easy change.
>
> The checked casts effectively mark the limits of where we've been able to
> push the RTL typesafety work.  Long term as we push the typesafety work
> further into the compiler many/most of the checked casts will go away.
>
> Unfortunately, that won't work in this case because other code wants to
> store a (const0_rtx) into the insn list.  (const0_rtx) isn't an INSN, so the
> checked cast fails and we get a nice abort/ICE.
>
> Conceptually we just need another marker that is an INSN and we might as
> well just convert the whole file to use the new interface at that point.
>
> Consider the request pulled.
>
> The const0-rtx problem may be why this wasn't converted in the first palce.
> Or it may simply have been a time problem.  David's done > 250 patches
> around RTL typesafety, but he also has other work to be doing ;-)
>
>>
>>   4. This bug is only reproduceable with my local customized GCC
>> version. So I don't have a testcase then.
>
> OK.
>
> I'll do a final review when I get notice about the copyright assignment from
> the FSF.
>
> jeff
>
Index: gcc/ChangeLog
===
--- gcc/ChangeLog   (revision 215658)
+++ gcc/ChangeLog   (working copy)
@@ -1,3 +1,14 @@
+2014-09-26  Felix Yang  
+   Jeff Law  
+
+   * ira.c (struct equivalence): Change member "is_arg_equivalence" and 
"replace"
+   into boolean bitfields; turn member "loop_depth" into a short int

Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-26 Thread Jeff Law

On 09/26/14 07:57, Felix Yang wrote:

Hi Jeff,

 Thanks for the suggestions. I updated the patch accordingly.

 1. Both my employer(Huawei) and I have signed the copyright
assignments with FSF.
 These assignments are already sent via post two days ago and
hopefully should reach FSF in one week.
 Maybe it's OK to commit this patch now?
Not really.  It needs to be accepted by the FSF before we can include 
the work.




  2. I am not turning member loop_depth of struct equivalence into
short integer as GCC API such as bb_loop_depth
  returns a loop's depth as a 32-bit interger.
There's already other places that assume loops don't nest that deep. 
Please go ahead and change it.  And no need to explicitly mark the 
unused bits.  That's just a maintenance nightmare in the long term 
anyway :-)





  3. I find it's kind of difficult to use the new type and
interfaces for list walking the init_insns list for this patch.
 The type of init_insns list is rtx, not rtl_insn_list *. Seems
we need to change a lot in order to use the new interface.
 Not clear about the reason why it is not adjusted when we are
transferring to the new interface.
 Anyway, I think it's better to have another patch fix that issue. OK?
The right way to go is to add a checked cast when we have some code that 
is using the old interface and other code using the new interface.  It's 
actually a pretty easy change.


The checked casts effectively mark the limits of where we've been able 
to push the RTL typesafety work.  Long term as we push the typesafety 
work further into the compiler many/most of the checked casts will go away.


Unfortunately, that won't work in this case because other code wants to 
store a (const0_rtx) into the insn list.  (const0_rtx) isn't an INSN, so 
the checked cast fails and we get a nice abort/ICE.


Conceptually we just need another marker that is an INSN and we might as 
well just convert the whole file to use the new interface at that point.


Consider the request pulled.

The const0-rtx problem may be why this wasn't converted in the first 
palce.  Or it may simply have been a time problem.  David's done > 250 
patches around RTL typesafety, but he also has other work to be doing ;-)




  4. This bug is only reproduceable with my local customized GCC
version. So I don't have a testcase then.

OK.

I'll do a final review when I get notice about the copyright assignment 
from the FSF.


jeff



Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-26 Thread Felix Yang
Hi Jeff,

Thanks for the suggestions. I updated the patch accordingly.

1. Both my employer(Huawei) and I have signed the copyright
assignments with FSF.
These assignments are already sent via post two days ago and
hopefully should reach FSF in one week.
Maybe it's OK to commit this patch now?

 2. I am not turning member loop_depth of struct equivalence into
short integer as GCC API such as bb_loop_depth
 returns a loop's depth as a 32-bit interger.

 3. I find it's kind of difficult to use the new type and
interfaces for list walking the init_insns list for this patch.
The type of init_insns list is rtx, not rtl_insn_list *. Seems
we need to change a lot in order to use the new interface.
Not clear about the reason why it is not adjusted when we are
transferring to the new interface.
Anyway, I think it's better to have another patch fix that issue. OK?

 4. This bug is only reproduceable with my local customized GCC
version. So I don't have a testcase then.

 5. This patch bootstrapped on x86_64-suse-linux and reg-tested,
There are no regressions with this patch.
 Regression test summary with or without the patch:

=== gcc Summary ===

# of expected passes107986
# of unexpected failures348
# of unexpected successes33
# of expected failures262
# of unsupported tests2089
/home/yangfei/gcc-devel/gcc-build/gcc/xgcc  version 5.0.0 20140924
(experimental) (GCC)
--
=== g++ Summary ===

# of expected passes87415
# of unexpected failures276
# of expected failures266
# of unsupported tests3203
/home/yangfei/gcc-devel/gcc-build/gcc/testsuite/g++/../../xg++
version 5.0.0 20140924 (experimental) (GCC)

--
=== libatomic Summary ===

# of expected passes54
=== libgomp tests ===


Running target unix

=== libgomp Summary ===

# of expected passes693
=== libitm tests ===


Running target unix

=== libitm Summary ===

# of expected passes26
# of expected failures3
# of unsupported tests1
=== libstdc++ tests ===


+++ gcc/ChangeLog(working copy)
@@ -1,3 +1,13 @@
+2014-09-26  Felix Yang  
+Jeff Law  
+
+* ira.c (struct equivalence): Change member "is_arg_equivalence"
and "replace"
+into boolean bitfields; add new member "no_equiv" and "reserved".
+(no_equiv): Set no_equiv of struct equivalence if register is marked
+as having no known equivalence.
+(update_equiv_regs): Check all definitions for a multiple-set
+register to make sure that the RHS have the same value.
+
 2014-09-26  Martin Liska  

 * cgraph.c (cgraph_node::release_body): New argument keep_arguments
Index: gcc/ira.c
===
--- gcc/ira.c(revision 215640)
+++ gcc/ira.c(working copy)
@@ -2896,10 +2896,13 @@ struct equivalence
  to be present within the same loop (or in an inner loop).  */
   int loop_depth;
   /* Nonzero if this had a preexisting REG_EQUIV note.  */
-  int is_arg_equivalence;
+  unsigned char is_arg_equivalence : 1;
   /* Set when an attempt should be made to replace a register
  with the associated src_p entry.  */
-  char replace;
+  unsigned char replace : 1;
+  /* Set if this register has no known equivalence.  */
+  unsigned char no_equiv : 1;
+  unsigned char reserved : 5;
 };

 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3247,6 +3250,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
 return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list == const0_rtx)
 return;
@@ -3258,7 +3262,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
 return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = XEXP (list, 1))
 {
   rtx insn = XEXP (list, 0);
   remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
@@ -3373,7 +3377,7 @@ update_equiv_regs (void)

   /* If this insn contains more (or less) than a single SET,
  only mark all destinations as having no known equivalence.  */
-  if (set == 0)
+  if (set == NULL_RTX)
 {
   note_stores (PATTERN (insn), no_equiv, NULL);
   continue;
@@ -3467,16 +3471,48 @@ update_equiv_regs (void)
   if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 note = NULL_RTX;

-  if (DF_REG_DEF_COUNT (regno) != 1
-  && (! note
+  if (DF_REG_DEF_COUNT (regno) != 1)
+{
+  rtx list;
+  bool equal_p = true;
+
+  /* If we have already processed this pseudo and determined it
+ can not have an equivalence, then honor that decision.  */
+  if (reg_equiv[regno].no_equiv)
+continue;
+
+  if (! note
 

Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-25 Thread Jeff Law

On 09/24/14 06:07, Felix Yang wrote:

Hi Jeff,

 Thanks for the comments. I updated the patch adding some enhancements.
 Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.

 Three points:
 1. For multiple-set register, it is not qualified to have a equiv
note once it is marked by no_equiv. The patch is updated with
this consideration.

Correct.


 2. For the rtx_insn_list new interface, I noticed that the old
style XEXP accessor macros is still used in function no_equiv.
And I choose to the old style macros with this patch and should
come up with another patch to fix this issue, OK?
I'd rather any new code going in use the updated interfaces.  It's 
certainly OK to have af followup patch which converts more pre-existing 
code to the new interfaces.



 3. For the conditions that an insn on the init_insns list which
did not have a note, I reconsider this and find that this can
never happens. So I replaced the check with a gcc assertion.

OK.

Also, I should have asked this earlier, do you have an assignment on 
file with the FSF, or does your employer have any kind of blanket 
assignment on file with the FSF?  These changes are large enough to 
require an assignment.




Index: gcc/ira.c
===
--- gcc/ira.c(revision 215550)
+++ gcc/ira.c(working copy)
@@ -2900,6 +2900,8 @@ struct equivalence
/* Set when an attempt should be made to replace a register
   with the associated src_p entry.  */
char replace;
+  /* Set if this register has no known equivalence.  */
+  char no_equiv;
  };
As a follow-up, can you turn is_arg_equivalence, replace and no_equiv 
into boolean bitfields and turn loop_depth into a short (to match 
assumptions elsewhere in GCC).



The point is to get better packing of these objects and ultimately use 
less memory.



+
+  /* Check if it is possible that this multiple-set register has
+ a known equivalence.  */
+  if (reg_equiv[regno].no_equiv)
+continue;

This comment is a bit confusing.  Please consider something like

/* If we have already processed this pseudo and determined it
   can not have an equivalence, then honor that decision.  */


Do you have a testcase we can add to the regression suite?  If at all 
possible please include one.An execution test would be best, but you 
could also scan the RTL for bogus REG_EQUIV notes.


Please update to use the new type and interfaces for list walking the 
init_insns list.


Finally, you need to verify your patch will bootstrap and not cause any 
regressions in the testsuite.  If you're unsure how to do that, let me know.


I think we'll be ready to go once those tasks are complete.


jeff



Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-24 Thread Felix Yang
PING ?
Cheers,
Felix


On Wed, Sep 24, 2014 at 8:07 PM, Felix Yang  wrote:
> Hi Jeff,
>
> Thanks for the comments. I updated the patch adding some enhancements.
> Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for 
> trunk.
>
> Three points:
> 1. For multiple-set register, it is not qualified to have a equiv
> note once it is marked by no_equiv. The patch is updated with
>this consideration.
> 2. For the rtx_insn_list new interface, I noticed that the old
> style XEXP accessor macros is still used in function no_equiv.
>And I choose to the old style macros with this patch and should
> come up with another patch to fix this issue, OK?
> 3. For the conditions that an insn on the init_insns list which
> did not have a note, I reconsider this and find that this can
>never happens. So I replaced the check with a gcc assertion.
>
>
> Index: gcc/ChangeLog
> ===
> --- gcc/ChangeLog(revision 215550)
> +++ gcc/ChangeLog(working copy)
> @@ -1,3 +1,11 @@
> +2014-09-24  Felix Yang  
> +
> +* ira.c (struct equivalence): Add no_equiv member.
> +(no_equiv): Set no_equiv of struct equivalence if register is marked
> +as having no known equivalence.
> +(update_equiv_regs): Check all definitions for a multiple-set
> +register to make sure that the RHS have the same value.
> +
>  2014-09-24  Jakub Jelinek  
>
>  PR sanitizer/63316
> Index: gcc/ira.c
> ===
> --- gcc/ira.c(revision 215550)
> +++ gcc/ira.c(working copy)
> @@ -2900,6 +2900,8 @@ struct equivalence
>/* Set when an attempt should be made to replace a register
>   with the associated src_p entry.  */
>char replace;
> +  /* Set if this register has no known equivalence.  */
> +  char no_equiv;
>  };
>
>  /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
> @@ -3247,6 +3249,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>if (!REG_P (reg))
>  return;
>regno = REGNO (reg);
> +  reg_equiv[regno].no_equiv = 1;
>list = reg_equiv[regno].init_insns;
>if (list == const0_rtx)
>  return;
> @@ -3258,7 +3261,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
>  return;
>ira_reg_equiv[regno].defined_p = false;
>ira_reg_equiv[regno].init_insns = NULL;
> -  for (; list; list =  XEXP (list, 1))
> +  for (; list; list = XEXP (list, 1))
>  {
>rtx insn = XEXP (list, 0);
>remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
> @@ -3373,7 +3376,7 @@ update_equiv_regs (void)
>
>/* If this insn contains more (or less) than a single SET,
>   only mark all destinations as having no known equivalence.  */
> -  if (set == 0)
> +  if (set == NULL_RTX)
>  {
>note_stores (PATTERN (insn), no_equiv, NULL);
>continue;
> @@ -3467,16 +3470,48 @@ update_equiv_regs (void)
>if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
>  note = NULL_RTX;
>
> -  if (DF_REG_DEF_COUNT (regno) != 1
> -  && (! note
> +  if (DF_REG_DEF_COUNT (regno) != 1)
> +{
> +  rtx list;
> +  bool equal_p = true;
> +
> +  /* Check if it is possible that this multiple-set register has
> + a known equivalence.  */
> +  if (reg_equiv[regno].no_equiv)
> +continue;
> +
> +  if (! note
>|| rtx_varies_p (XEXP (note, 0), 0)
>|| (reg_equiv[regno].replacement
>&& ! rtx_equal_p (XEXP (note, 0),
> -reg_equiv[regno].replacement
> -{
> -  no_equiv (dest, set, NULL);
> -  continue;
> +reg_equiv[regno].replacement)))
> +{
> +  no_equiv (dest, set, NULL);
> +  continue;
> +}
> +
> +  list = reg_equiv[regno].init_insns;
> +  for (; list; list = XEXP (list, 1))
> +{
> +  rtx note_tmp, insn_tmp;
> +
> +  insn_tmp = XEXP (list, 0);
> +  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
> +  gcc_assert (note_tmp);
> +  if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
> +{
> +  equal_p = false;
> +  break;
> +}
> +}
> +
> +  if (! equal_p)
> +{
> +  no_equiv (dest, set, NULL);
> +  continue;
> +}
>  }
> +
>/* Record this insn as initializing this register.  */
>reg_equiv[regno].init_insns
>  = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
> @@ -3505,10 +3540,9 @@ update_equiv_regs (void)
>   a register used only in one basic block from a MEM.  If so, and the
>   MEM remains unchanged for the life of the register, add a REG_EQUIV
>   note.  */
> -
>note = find_reg_note (insn, REG

Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-24 Thread Felix Yang
Hi Jeff,

Thanks for the comments. I updated the patch adding some enhancements.
Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.

Three points:
1. For multiple-set register, it is not qualified to have a equiv
note once it is marked by no_equiv. The patch is updated with
   this consideration.
2. For the rtx_insn_list new interface, I noticed that the old
style XEXP accessor macros is still used in function no_equiv.
   And I choose to the old style macros with this patch and should
come up with another patch to fix this issue, OK?
3. For the conditions that an insn on the init_insns list which
did not have a note, I reconsider this and find that this can
   never happens. So I replaced the check with a gcc assertion.


Index: gcc/ChangeLog
===
--- gcc/ChangeLog(revision 215550)
+++ gcc/ChangeLog(working copy)
@@ -1,3 +1,11 @@
+2014-09-24  Felix Yang  
+
+* ira.c (struct equivalence): Add no_equiv member.
+(no_equiv): Set no_equiv of struct equivalence if register is marked
+as having no known equivalence.
+(update_equiv_regs): Check all definitions for a multiple-set
+register to make sure that the RHS have the same value.
+
 2014-09-24  Jakub Jelinek  

 PR sanitizer/63316
Index: gcc/ira.c
===
--- gcc/ira.c(revision 215550)
+++ gcc/ira.c(working copy)
@@ -2900,6 +2900,8 @@ struct equivalence
   /* Set when an attempt should be made to replace a register
  with the associated src_p entry.  */
   char replace;
+  /* Set if this register has no known equivalence.  */
+  char no_equiv;
 };

 /* reg_equiv[N] (where N is a pseudo reg number) is the equivalence
@@ -3247,6 +3249,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
   if (!REG_P (reg))
 return;
   regno = REGNO (reg);
+  reg_equiv[regno].no_equiv = 1;
   list = reg_equiv[regno].init_insns;
   if (list == const0_rtx)
 return;
@@ -3258,7 +3261,7 @@ no_equiv (rtx reg, const_rtx store ATTRIBUTE_UNUSE
 return;
   ira_reg_equiv[regno].defined_p = false;
   ira_reg_equiv[regno].init_insns = NULL;
-  for (; list; list =  XEXP (list, 1))
+  for (; list; list = XEXP (list, 1))
 {
   rtx insn = XEXP (list, 0);
   remove_note (insn, find_reg_note (insn, REG_EQUIV, NULL_RTX));
@@ -3373,7 +3376,7 @@ update_equiv_regs (void)

   /* If this insn contains more (or less) than a single SET,
  only mark all destinations as having no known equivalence.  */
-  if (set == 0)
+  if (set == NULL_RTX)
 {
   note_stores (PATTERN (insn), no_equiv, NULL);
   continue;
@@ -3467,16 +3470,48 @@ update_equiv_regs (void)
   if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 note = NULL_RTX;

-  if (DF_REG_DEF_COUNT (regno) != 1
-  && (! note
+  if (DF_REG_DEF_COUNT (regno) != 1)
+{
+  rtx list;
+  bool equal_p = true;
+
+  /* Check if it is possible that this multiple-set register has
+ a known equivalence.  */
+  if (reg_equiv[regno].no_equiv)
+continue;
+
+  if (! note
   || rtx_varies_p (XEXP (note, 0), 0)
   || (reg_equiv[regno].replacement
   && ! rtx_equal_p (XEXP (note, 0),
-reg_equiv[regno].replacement
-{
-  no_equiv (dest, set, NULL);
-  continue;
+reg_equiv[regno].replacement)))
+{
+  no_equiv (dest, set, NULL);
+  continue;
+}
+
+  list = reg_equiv[regno].init_insns;
+  for (; list; list = XEXP (list, 1))
+{
+  rtx note_tmp, insn_tmp;
+
+  insn_tmp = XEXP (list, 0);
+  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+  gcc_assert (note_tmp);
+  if (! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+{
+  equal_p = false;
+  break;
+}
+}
+
+  if (! equal_p)
+{
+  no_equiv (dest, set, NULL);
+  continue;
+}
 }
+
   /* Record this insn as initializing this register.  */
   reg_equiv[regno].init_insns
 = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
@@ -3505,10 +3540,9 @@ update_equiv_regs (void)
  a register used only in one basic block from a MEM.  If so, and the
  MEM remains unchanged for the life of the register, add a REG_EQUIV
  note.  */
-
   note = find_reg_note (insn, REG_EQUIV, NULL_RTX);

-  if (note == 0 && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
+  if (note == NULL_RTX && REG_BASIC_BLOCK (regno) >= NUM_FIXED_BLOCKS
   && MEM_P (SET_SRC (set))
   && validate_equiv_mem (insn, dest, SET_SRC (set)))
 note = set_unique_reg_note (insn, REG_EQUIV, copy_rtx (SET_SRC (set)

Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-23 Thread Jeff Law

On 09/23/14 04:51, Felix Yang wrote:

Hi,

 Ignore the previous message.
 Attached please find the updated patch.
 Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.

Index: gcc/ChangeLog
===
--- gcc/ChangeLog(revision 215500)
+++ gcc/ChangeLog(working copy)
@@ -1,3 +1,8 @@
+2014-09-23  Felix Yang  
+
+* ira.c (update_equiv_regs): Check all definitions for a multiple-set
+register to make sure that the RHS have the same value.
+
  2014-09-23  Ilya Enkovich  

  * cfgcleanup.c (try_optimize_cfg): Do not remove label
Index: gcc/ira.c
===
--- gcc/ira.c(revision 215500)
+++ gcc/ira.c(working copy)
@@ -3467,16 +3467,43 @@ update_equiv_regs (void)
if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
  note = NULL_RTX;

-  if (DF_REG_DEF_COUNT (regno) != 1
-  && (! note
+  if (DF_REG_DEF_COUNT (regno) != 1)
+{
+  rtx list;

This should probably be "rtx_insn_list list".


+  list = reg_equiv[regno].init_insns;
+  for (; list; list = XEXP (list, 1))
Please use the next/insn member functions of the rtx_insn_list rather 
than the old style XEXP accessor macros.




+{
+  rtx note_tmp, insn_tmp;
+  insn_tmp = XEXP (list, 0);
+  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+
+  if (note_tmp == 0
+  || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
Under what conditions did you find an insn on this list that did not 
have a note?  There's a deeper question I'm getting to, but let's start 
here.


Rather than use "== 0", if you have an RTX use "== NULL_RTX".  That 
applies to the note_tmp == 0 test above.



Can you make the edits noted above & answer the question WRT insns on 
the reg_equiv[].init_insns list without notes and repost?


Thanks,
Jeff


Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-23 Thread Felix Yang
Hi,

Ignore the previous message.
Attached please find the updated patch.
Bootstrapped on x86_64-suse-linux. Please apply this patch if OK for trunk.

Index: gcc/ChangeLog
===
--- gcc/ChangeLog(revision 215500)
+++ gcc/ChangeLog(working copy)
@@ -1,3 +1,8 @@
+2014-09-23  Felix Yang  
+
+* ira.c (update_equiv_regs): Check all definitions for a multiple-set
+register to make sure that the RHS have the same value.
+
 2014-09-23  Ilya Enkovich  

 * cfgcleanup.c (try_optimize_cfg): Do not remove label
Index: gcc/ira.c
===
--- gcc/ira.c(revision 215500)
+++ gcc/ira.c(working copy)
@@ -3467,16 +3467,43 @@ update_equiv_regs (void)
   if (note && GET_CODE (XEXP (note, 0)) == EXPR_LIST)
 note = NULL_RTX;

-  if (DF_REG_DEF_COUNT (regno) != 1
-  && (! note
+  if (DF_REG_DEF_COUNT (regno) != 1)
+{
+  rtx list;
+  bool equal_p = true;
+
+  if (! note
   || rtx_varies_p (XEXP (note, 0), 0)
   || (reg_equiv[regno].replacement
   && ! rtx_equal_p (XEXP (note, 0),
-reg_equiv[regno].replacement
-{
-  no_equiv (dest, set, NULL);
-  continue;
+reg_equiv[regno].replacement)))
+{
+  no_equiv (dest, set, NULL);
+  continue;
+}
+
+  list = reg_equiv[regno].init_insns;
+  for (; list; list = XEXP (list, 1))
+{
+  rtx note_tmp, insn_tmp;
+  insn_tmp = XEXP (list, 0);
+  note_tmp = find_reg_note (insn_tmp, REG_EQUAL, NULL_RTX);
+
+  if (note_tmp == 0
+  || ! rtx_equal_p (XEXP (note, 0), XEXP (note_tmp, 0)))
+{
+  equal_p = false;
+  break;
+}
+}
+
+  if (! equal_p)
+{
+  no_equiv (dest, set, NULL);
+  continue;
+}
 }
+
   /* Record this insn as initializing this register.  */
   reg_equiv[regno].init_insns
 = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv[regno].init_insns);
Cheers,
Felix


On Tue, Sep 23, 2014 at 6:45 PM, Felix Yang  wrote:
> Hi,
> Attached please fined the updated patch.
>
> Cheers,
> Felix
>
>
> On Tue, Sep 23, 2014 at 10:48 AM, Yangfei (Felix)  
> wrote:
>>> On 09/22/14 08:40, Felix Yang wrote:
>>> > Hi,
>>> >
>>> >  I find that update_equiv_regs in ira.c sets the wrong EQUIV
>>> > reg-note for pseudo with more than one definiton in certain situation.
>>> >  Here is a simplified RTL snippet after this function finishs 
>>> > handling:
>>> >
>>> >   (insn 77 37 78 2 (set (reg:SI 171)
>>> >   (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
>>> >(expr_list:REG_EQUAL (const_int 0 [0])
>>> >   (nil)))
>>> >
>>> > ..
>>> >
>>> > (insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
>>> >   (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
>>> >(expr_list:REG_DEAD (reg:SI 171)
>>> >   (nil)))
>>> > (insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
>>> >   (reg:SI 163)) 52 {movsi_internal_dsp}
>>> >(expr_list:REG_DEAD (reg:SI 163)
>>> >   (expr_list:REG_DEAD (reg/f:SI 162)
>>> >   (nil
>>> > (insn 54 53 14 2 (set (reg:SI 171)
>>> >   (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
>>> > A32])) ticket151.c:49 52 {movsi_internal_dsp}
>>> >(expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
>>> > [flags 0x2]) [4  S4 A32])
>>> >   (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
>>> > [flags 0x2]) [4  S4 A32])
>>> >   (nil
>>> >
>>> >
>>> >  The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
>>> > in insn 77 with a differerent value.
>>> >  This may causes reload replacing pseudo 171 with mem/u/c:SI
>>> > (symbol_ref/u:SI ("*.LC8"), which is wrong.
>>> >  A proposed patch for this issue, please comment:
>>> Is it possible it's the conditional just above this one that is incorrect?
>>>
>>>   if (DF_REG_DEF_COUNT (regno) != 1
>>>&& (! note
>>>|| rtx_varies_p (XEXP (note, 0), 0)
>>>|| (reg_equiv[regno].replacement
>>>&& ! rtx_equal_p (XEXP (note, 0),
>>>
>>> reg_equiv[regno].replacement
>>>  {
>>>no_equiv (dest, set, NULL);
>>>continue;
>>>  }
>>>
>>>
>>> ISTM the only time a multiply-set register can have a valid REG_EQUIV note 
>>> is if
>>> each and every assignment to that pseudo has the same RHS.
>>>
>>> Jeff
>>
>>
>> Thanks Jeff. Yes, I agree that this is a better place to consider. I am 
>> thinking of a better way to fix this.
>>
>> Vladimir, can you shed light on this and give me some suggestions? Than

Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-23 Thread Felix Yang
Hi,
Attached please fined the updated patch.

Cheers,
Felix


On Tue, Sep 23, 2014 at 10:48 AM, Yangfei (Felix)  wrote:
>> On 09/22/14 08:40, Felix Yang wrote:
>> > Hi,
>> >
>> >  I find that update_equiv_regs in ira.c sets the wrong EQUIV
>> > reg-note for pseudo with more than one definiton in certain situation.
>> >  Here is a simplified RTL snippet after this function finishs handling:
>> >
>> >   (insn 77 37 78 2 (set (reg:SI 171)
>> >   (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
>> >(expr_list:REG_EQUAL (const_int 0 [0])
>> >   (nil)))
>> >
>> > ..
>> >
>> > (insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
>> >   (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
>> >(expr_list:REG_DEAD (reg:SI 171)
>> >   (nil)))
>> > (insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
>> >   (reg:SI 163)) 52 {movsi_internal_dsp}
>> >(expr_list:REG_DEAD (reg:SI 163)
>> >   (expr_list:REG_DEAD (reg/f:SI 162)
>> >   (nil
>> > (insn 54 53 14 2 (set (reg:SI 171)
>> >   (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
>> > A32])) ticket151.c:49 52 {movsi_internal_dsp}
>> >(expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
>> > [flags 0x2]) [4  S4 A32])
>> >   (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
>> > [flags 0x2]) [4  S4 A32])
>> >   (nil
>> >
>> >
>> >  The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
>> > in insn 77 with a differerent value.
>> >  This may causes reload replacing pseudo 171 with mem/u/c:SI
>> > (symbol_ref/u:SI ("*.LC8"), which is wrong.
>> >  A proposed patch for this issue, please comment:
>> Is it possible it's the conditional just above this one that is incorrect?
>>
>>   if (DF_REG_DEF_COUNT (regno) != 1
>>&& (! note
>>|| rtx_varies_p (XEXP (note, 0), 0)
>>|| (reg_equiv[regno].replacement
>>&& ! rtx_equal_p (XEXP (note, 0),
>>
>> reg_equiv[regno].replacement
>>  {
>>no_equiv (dest, set, NULL);
>>continue;
>>  }
>>
>>
>> ISTM the only time a multiply-set register can have a valid REG_EQUIV note 
>> is if
>> each and every assignment to that pseudo has the same RHS.
>>
>> Jeff
>
>
> Thanks Jeff. Yes, I agree that this is a better place to consider. I am 
> thinking of a better way to fix this.
>
> Vladimir, can you shed light on this and give me some suggestions? Thank you.
>


Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-22 Thread Yangfei (Felix)
> On 09/22/14 08:40, Felix Yang wrote:
> > Hi,
> >
> >  I find that update_equiv_regs in ira.c sets the wrong EQUIV
> > reg-note for pseudo with more than one definiton in certain situation.
> >  Here is a simplified RTL snippet after this function finishs handling:
> >
> >   (insn 77 37 78 2 (set (reg:SI 171)
> >   (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
> >(expr_list:REG_EQUAL (const_int 0 [0])
> >   (nil)))
> >
> > ..
> >
> > (insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
> >   (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
> >(expr_list:REG_DEAD (reg:SI 171)
> >   (nil)))
> > (insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
> >   (reg:SI 163)) 52 {movsi_internal_dsp}
> >(expr_list:REG_DEAD (reg:SI 163)
> >   (expr_list:REG_DEAD (reg/f:SI 162)
> >   (nil
> > (insn 54 53 14 2 (set (reg:SI 171)
> >   (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
> > A32])) ticket151.c:49 52 {movsi_internal_dsp}
> >(expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
> > [flags 0x2]) [4  S4 A32])
> >   (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
> > [flags 0x2]) [4  S4 A32])
> >   (nil
> >
> >
> >  The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
> > in insn 77 with a differerent value.
> >  This may causes reload replacing pseudo 171 with mem/u/c:SI
> > (symbol_ref/u:SI ("*.LC8"), which is wrong.
> >  A proposed patch for this issue, please comment:
> Is it possible it's the conditional just above this one that is incorrect?
> 
>   if (DF_REG_DEF_COUNT (regno) != 1
>&& (! note
>|| rtx_varies_p (XEXP (note, 0), 0)
>|| (reg_equiv[regno].replacement
>&& ! rtx_equal_p (XEXP (note, 0),
> 
> reg_equiv[regno].replacement
>  {
>no_equiv (dest, set, NULL);
>continue;
>  }
> 
> 
> ISTM the only time a multiply-set register can have a valid REG_EQUIV note is 
> if
> each and every assignment to that pseudo has the same RHS.
> 
> Jeff


Thanks Jeff. Yes, I agree that this is a better place to consider. I am 
thinking of a better way to fix this.

Vladimir, can you shed light on this and give me some suggestions? Thank you.



Re: [PATCH IRA] update_equiv_regs fails to set EQUIV reg-note for pseudo with more than one definition

2014-09-22 Thread Jeff Law

On 09/22/14 08:40, Felix Yang wrote:

Hi,

 I find that update_equiv_regs in ira.c sets the wrong EQUIV
reg-note for pseudo with more than one definiton in certain situation.
 Here is a simplified RTL snippet after this function finishs handling:

  (insn 77 37 78 2 (set (reg:SI 171)
  (const_int 0 [0])) ticket151.c:33 52 {movsi_internal_dsp}
   (expr_list:REG_EQUAL (const_int 0 [0])
  (nil)))

..

(insn 79 50 53 2 (set (mem/c:SI (reg/f:SI 136) [2 g_728+0 S4 A64])
  (reg:SI 171)) ticket151.c:33 52 {movsi_internal_dsp}
   (expr_list:REG_DEAD (reg:SI 171)
  (nil)))
(insn 53 79 54 2 (set (mem/c:SI (reg/f:SI 162) [4 g_163+0 S4 A32])
  (reg:SI 163)) 52 {movsi_internal_dsp}
   (expr_list:REG_DEAD (reg:SI 163)
  (expr_list:REG_DEAD (reg/f:SI 162)
  (nil
(insn 54 53 14 2 (set (reg:SI 171)
  (mem/u/c:SI (symbol_ref/u:SI ("*.LC8") [flags 0x2]) [4  S4
A32])) ticket151.c:49 52 {movsi_internal_dsp}
   (expr_list:REG_EQUIV (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
[flags 0x2]) [4  S4 A32])
  (expr_list:REG_EQUAL (mem/u/c:SI (symbol_ref/u:SI ("*.LC8")
[flags 0x2]) [4  S4 A32])
  (nil


 The REG_EQUIV of insn 54 is not correct as pseudo 171 is defined
in insn 77 with a differerent value.
 This may causes reload replacing pseudo 171 with mem/u/c:SI
(symbol_ref/u:SI ("*.LC8"), which is wrong.
 A proposed patch for this issue, please comment:

Is it possible it's the conditional just above this one that is incorrect?

 if (DF_REG_DEF_COUNT (regno) != 1
  && (! note
  || rtx_varies_p (XEXP (note, 0), 0)
  || (reg_equiv[regno].replacement
  && ! rtx_equal_p (XEXP (note, 0),
reg_equiv[regno].replacement
{
  no_equiv (dest, set, NULL);
  continue;
}


ISTM the only time a multiply-set register can have a valid REG_EQUIV 
note is if each and every assignment to that pseudo has the same RHS.


Jeff