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

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.

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;
>

On 2014-06-11, 1:17 PM, 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.
> 

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.
>

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.
> > 
> 
>

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.
>>>
>>
>>