[ARM] Fix build failure due to movsi_compare0

[ARM] Fix build failure due to movsi_compare0

[James Greenhalgh]

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


Hi,

A recent change somewhere exposed a latent bug between LRA and the definition
of the movsi_compare0 pattern.

This pattern ties the source and destination register of a set together
a (match_dup) and register constraints:

   [(set (reg:CC CC_REGNUM)
	(compare:CC (match_operand:SI 1 "s_register_operand" "0,r")
		    (const_int 0)))
    (set (match_operand:SI 0 "s_register_operand" "=r,r")
	(match_dup 1))]

This confuses LRA which expects the source and destination register of
a set to be different.

reduced.c: In function '_IO_vfscanf_internal':
reduced.c:104:1: internal compiler error: in lra_create_copy, at lra.c:1512
     }
 ^
0x8c3f9a lra_create_copy(int, int, int)
	/work/gcc-dev/src/gcc/gcc/lra.c:1512
0x8e4ab0 process_bb_lives
	/work/gcc-dev/src/gcc/gcc/lra-lives.c:568
0x8e4ab0 lra_create_live_ranges(bool)
	/work/gcc-dev/src/gcc/gcc/lra-lives.c:1019
0x8c5a39 lra(_IO_FILE*)
	/work/gcc-dev/src/gcc/gcc/lra.c:2356
0x873a96 do_reload
	/work/gcc-dev/src/gcc/gcc/ira.c:5415
0x873a96 execute
	/work/gcc-dev/src/gcc/gcc/ira.c:5576
Please submit a full bug report,

We can fix the pattern by moving away from match_dup and using register
tying with constraints consistently.

I'm not entirely convinced that this is legitimate (my vague recollection is
that register tying should only be used to tie inputs to outputs).

This has passed testing on a bunch of ARM targets, and fixes the build
issues I've been seeing.

OK for trunk?

Thanks,
James

---
gcc/

2014-06-11  James Greenhalgh  <james.greenhalgh@arm.com>

	* config/arm/arm.md (movsi_compare0): Clarify intentions using
	register tying.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ARM-Fix-build-failure-due-to-movsi_compare0.patch --]
[-- Type: text/x-patch;  name=0001-ARM-Fix-build-failure-due-to-movsi_compare0.patch, Size: 587 bytes --]

diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
index f58a79b..a01333b 100644
--- a/gcc/config/arm/arm.md
+++ b/gcc/config/arm/arm.md
@@ -6582,10 +6582,10 @@
 
 (define_insn "*movsi_compare0"
   [(set (reg:CC CC_REGNUM)
-	(compare:CC (match_operand:SI 1 "s_register_operand" "0,r")
-		    (const_int 0)))
+	(compare:CC (match_operand:SI 2 "s_register_operand" "0,1")
+	    (const_int 0)))
    (set (match_operand:SI 0 "s_register_operand" "=r,r")
-	(match_dup 1))]
+	(match_operand:SI 1 "s_register_operand" "0,r"))]
   "TARGET_32BIT"
   "@
    cmp%?\\t%0, #0

Re: [ARM] Fix build failure due to movsi_compare0 (PR 61430)

[Chung-Lin Tang]

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

On 2014/6/11 下午 06:32, James Greenhalgh wrote:
> 
> Hi,
> 
> A recent change somewhere exposed a latent bug between LRA and the definition
> of the movsi_compare0 pattern.
> 
> This pattern ties the source and destination register of a set together
> a (match_dup) and register constraints:
> 
>    [(set (reg:CC CC_REGNUM)
> 	(compare:CC (match_operand:SI 1 "s_register_operand" "0,r")
> 		    (const_int 0)))
>     (set (match_operand:SI 0 "s_register_operand" "=r,r")
> 	(match_dup 1))]
> 
> This confuses LRA which expects the source and destination register of
> a set to be different.
> 
> reduced.c: In function '_IO_vfscanf_internal':
> reduced.c:104:1: internal compiler error: in lra_create_copy, at lra.c:1512
>      }
>  ^
> 0x8c3f9a lra_create_copy(int, int, int)
> 	/work/gcc-dev/src/gcc/gcc/lra.c:1512
> 0x8e4ab0 process_bb_lives
> 	/work/gcc-dev/src/gcc/gcc/lra-lives.c:568
> 0x8e4ab0 lra_create_live_ranges(bool)
> 	/work/gcc-dev/src/gcc/gcc/lra-lives.c:1019
> 0x8c5a39 lra(_IO_FILE*)
> 	/work/gcc-dev/src/gcc/gcc/lra.c:2356
> 0x873a96 do_reload
> 	/work/gcc-dev/src/gcc/gcc/ira.c:5415
> 0x873a96 execute
> 	/work/gcc-dev/src/gcc/gcc/ira.c:5576
> Please submit a full bug report,
> 
> We can fix the pattern by moving away from match_dup and using register
> tying with constraints consistently.
> 
> I'm not entirely convinced that this is legitimate (my vague recollection is
> that register tying should only be used to tie inputs to outputs).
> 
> This has passed testing on a bunch of ARM targets, and fixes the build
> issues I've been seeing.

Looking at this too, as an LRA exercise. I don't really think the
pattern is wrong, rather LRA should just avoid creating the copy in this
case; it's a result of operand constraining, after all.

Attached is the small LRA patch, pending testing. Vladimir should weight
in on this.

Thanks,
Chung-Lin

	* ira-lives.c (process_bb_lives): Skip creating copy during
	insn sca when src/dest has constrained to same regno.


[-- Attachment #2: x.diff --]
[-- Type: text/plain, Size: 724 bytes --]

Index: lra-lives.c
===================================================================
--- lra-lives.c	(revision 211398)
+++ lra-lives.c	(working copy)
@@ -558,7 +558,11 @@ process_bb_lives (basic_block bb, int &curr_point)
 	      /* It might be 'inheritance pseudo <- reload pseudo'.  */
 	      || (src_regno >= lra_constraint_new_regno_start
 		  && ((int) REGNO (SET_DEST (set))
-		      >= lra_constraint_new_regno_start))))
+		      >= lra_constraint_new_regno_start)
+		  /* Remember to skip special cases where src/dest regnos are
+		     the same, e.g. insn SET pattern has matching constraints
+		     like =r,0.  */
+		  && src_regno != (int) REGNO (SET_DEST (set)))))
 	{
 	  int hard_regno = -1, regno = -1;
 

Re: [ARM] Fix build failure due to movsi_compare0 (PR 61430)

[James Greenhalgh]

On Wed, Jun 11, 2014 at 06:17:47PM +0100, Chung-Lin Tang wrote:
> On 2014/6/11 下午 06:32, James Greenhalgh wrote:
> > 
> > Hi,
> > 
> > A recent change somewhere exposed a latent bug between LRA and the definition
> > of the movsi_compare0 pattern.
> > 
> > This pattern ties the source and destination register of a set together
> > a (match_dup) and register constraints:
> > 
> >    [(set (reg:CC CC_REGNUM)
> > 	(compare:CC (match_operand:SI 1 "s_register_operand" "0,r")
> > 		    (const_int 0)))
> >     (set (match_operand:SI 0 "s_register_operand" "=r,r")
> > 	(match_dup 1))]
> > 
> > This confuses LRA which expects the source and destination register of
> > a set to be different.
> > 
> > reduced.c: In function '_IO_vfscanf_internal':
> > reduced.c:104:1: internal compiler error: in lra_create_copy, at lra.c:1512
> >      }
> >  ^
> > 0x8c3f9a lra_create_copy(int, int, int)
> > 	/work/gcc-dev/src/gcc/gcc/lra.c:1512
> > 0x8e4ab0 process_bb_lives
> > 	/work/gcc-dev/src/gcc/gcc/lra-lives.c:568
> > 0x8e4ab0 lra_create_live_ranges(bool)
> > 	/work/gcc-dev/src/gcc/gcc/lra-lives.c:1019
> > 0x8c5a39 lra(_IO_FILE*)
> > 	/work/gcc-dev/src/gcc/gcc/lra.c:2356
> > 0x873a96 do_reload
> > 	/work/gcc-dev/src/gcc/gcc/ira.c:5415
> > 0x873a96 execute
> > 	/work/gcc-dev/src/gcc/gcc/ira.c:5576
> > Please submit a full bug report,
> > 
> > We can fix the pattern by moving away from match_dup and using register
> > tying with constraints consistently.
> > 
> > I'm not entirely convinced that this is legitimate (my vague recollection is
> > that register tying should only be used to tie inputs to outputs).
> > 
> > This has passed testing on a bunch of ARM targets, and fixes the build
> > issues I've been seeing.
> 
> Looking at this too, as an LRA exercise. I don't really think the
> pattern is wrong, rather LRA should just avoid creating the copy in this
> case; it's a result of operand constraining, after all.
> 
> Attached is the small LRA patch, pending testing. Vladimir should weight
> in on this.

This is the better approach if the original pattern is legitimate.

I've tested this with ARM bootstrap and regression run with no issues.

Vlad, is this OK for trunk?

Thanks,
James

> 
> Thanks,
> Chung-Lin
> 
> 	* ira-lives.c (process_bb_lives): Skip creating copy during
> 	insn sca when src/dest has constrained to same regno.

> Index: lra-lives.c
> ===================================================================
> --- lra-lives.c	(revision 211398)
> +++ lra-lives.c	(working copy)
> @@ -558,7 +558,11 @@ process_bb_lives (basic_block bb, int &curr_point)
>  	      /* It might be 'inheritance pseudo <- reload pseudo'.  */
>  	      || (src_regno >= lra_constraint_new_regno_start
>  		  && ((int) REGNO (SET_DEST (set))
> -		      >= lra_constraint_new_regno_start))))
> +		      >= lra_constraint_new_regno_start)
> +		  /* Remember to skip special cases where src/dest regnos are
> +		     the same, e.g. insn SET pattern has matching constraints
> +		     like =r,0.  */
> +		  && src_regno != (int) REGNO (SET_DEST (set)))))
>  	{
>  	  int hard_regno = -1, regno = -1;
>  

Re: [ARM] Fix build failure due to movsi_compare0 (PR 61430)

[Vladimir Makarov]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=Big5, Size: 2147 bytes --]

On 2014-06-11, 1:17 PM, Chung-Lin Tang wrote:
> On 2014/6/11 ¤U¤È 06:32, James Greenhalgh wrote:
>>
>> Hi,
>>
>> A recent change somewhere exposed a latent bug between LRA and the definition
>> of the movsi_compare0 pattern.
>>
>> This pattern ties the source and destination register of a set together
>> a (match_dup) and register constraints:
>>
>>     [(set (reg:CC CC_REGNUM)
>> 	(compare:CC (match_operand:SI 1 "s_register_operand" "0,r")
>> 		    (const_int 0)))
>>      (set (match_operand:SI 0 "s_register_operand" "=r,r")
>> 	(match_dup 1))]
>>
>> This confuses LRA which expects the source and destination register of
>> a set to be different.
>>
>> reduced.c: In function '_IO_vfscanf_internal':
>> reduced.c:104:1: internal compiler error: in lra_create_copy, at lra.c:1512
>>       }
>>   ^
>> 0x8c3f9a lra_create_copy(int, int, int)
>> 	/work/gcc-dev/src/gcc/gcc/lra.c:1512
>> 0x8e4ab0 process_bb_lives
>> 	/work/gcc-dev/src/gcc/gcc/lra-lives.c:568
>> 0x8e4ab0 lra_create_live_ranges(bool)
>> 	/work/gcc-dev/src/gcc/gcc/lra-lives.c:1019
>> 0x8c5a39 lra(_IO_FILE*)
>> 	/work/gcc-dev/src/gcc/gcc/lra.c:2356
>> 0x873a96 do_reload
>> 	/work/gcc-dev/src/gcc/gcc/ira.c:5415
>> 0x873a96 execute
>> 	/work/gcc-dev/src/gcc/gcc/ira.c:5576
>> Please submit a full bug report,
>>
>> We can fix the pattern by moving away from match_dup and using register
>> tying with constraints consistently.
>>
>> I'm not entirely convinced that this is legitimate (my vague recollection is
>> that register tying should only be used to tie inputs to outputs).
>>
>> This has passed testing on a bunch of ARM targets, and fixes the build
>> issues I've been seeing.
> 
> Looking at this too, as an LRA exercise. I don't really think the
> pattern is wrong, rather LRA should just avoid creating the copy in this
> case; it's a result of operand constraining, after all.
> 
> Attached is the small LRA patch, pending testing. Vladimir should weight
> in on this.
> 

The patch is safe and ok.  Thanks for working on it, Chung-Lin.

> 	* ira-lives.c (process_bb_lives): Skip creating copy during
> 	insn sca when src/dest has constrained to same regno.
> 

Re: [ARM] Fix build failure due to movsi_compare0 (PR 61430)

[James Greenhalgh]

On Fri, Jun 13, 2014 at 05:46:45PM +0100, Vladimir Makarov wrote:
> On 2014-06-11, 1:17 PM, Chung-Lin Tang wrote:
> > Looking at this too, as an LRA exercise. I don't really think the
> > pattern is wrong, rather LRA should just avoid creating the copy in this
> > case; it's a result of operand constraining, after all.
> > 
> > Attached is the small LRA patch, pending testing. Vladimir should weight
> > in on this.
> > 
> 
> The patch is safe and ok.  Thanks for working on it, Chung-Lin.

As this patch fixes a build failure on ARM I'd like to have it applied
today. If I don't hear anything which would stop me, I'll commit this on
Chung-Lin's behalf in a few hours.

Cheers,
James

> 
> > 	* ira-lives.c (process_bb_lives): Skip creating copy during
> > 	insn sca when src/dest has constrained to same regno.
> > 
> 
> 

Re: [ARM] Fix build failure due to movsi_compare0 (PR 61430)

[Chung-Lin Tang]

On 14/6/16 5:55 PM, James Greenhalgh wrote:
> On Fri, Jun 13, 2014 at 05:46:45PM +0100, Vladimir Makarov wrote:
>> On 2014-06-11, 1:17 PM, Chung-Lin Tang wrote:
>>> Looking at this too, as an LRA exercise. I don't really think the
>>> pattern is wrong, rather LRA should just avoid creating the copy in this
>>> case; it's a result of operand constraining, after all.
>>>
>>> Attached is the small LRA patch, pending testing. Vladimir should weight
>>> in on this.
>>>
>>
>> The patch is safe and ok.  Thanks for working on it, Chung-Lin.
> 
> As this patch fixes a build failure on ARM I'd like to have it applied
> today. If I don't hear anything which would stop me, I'll commit this on
> Chung-Lin's behalf in a few hours.
> 
> Cheers,
> James

I just committed it, after a testsuite run on x86_64 (to be sure). And
thanks to James for doing the ARM tests.

Thanks,
Chung-Lin

>>
>>> 	* ira-lives.c (process_bb_lives): Skip creating copy during
>>> 	insn sca when src/dest has constrained to same regno.
>>>
>>
>>