[PATCH] Fix PR 44691

[PATCH] Fix PR 44691

[Andrey Belevantsev]

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

Hello,

As explained in the audit trail, the problem was that in the selective 
scheduler I assumed that SUBREG_REG will always be a REG, which seems to be 
not the case.  This is not quite in line with what documentation says, if I 
read it correctly, but it seems to be used in a number of backends, so the 
below patch just gives up substitution also when SUBREG_REG is not a 
register.  Bootstrapped and tested on ia64, and verified that the test is 
fixed on x86_64.

I think that this qualifies as obvious, so unless Vlad or other people have 
any comments, I'll commit it tomorrow.

Yours, Andrey

2010-08-18  Andrey Belevantsev  <abel@ispras.ru>

	PR rtl-optimization/44691

	* sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
	is not a register.


[-- Attachment #2: pr44691.diff --]
[-- Type: text/x-patch, Size: 858 bytes --]

Index: gcc/sel-sched.c
===================================================================
*** gcc/sel-sched.c	(revision 163192)
--- gcc/sel-sched.c	(working copy)
*************** count_occurrences_1 (rtx *cur_rtx, void 
*** 835,841 ****
  
    if (GET_CODE (*cur_rtx) == SUBREG
        && REG_P (p->x)
!       && REGNO (SUBREG_REG (*cur_rtx)) == REGNO (p->x))
      {
        /* ??? Do not support substituting regs inside subregs.  In that case,
           simplify_subreg will be called by validate_replace_rtx, and
--- 835,842 ----
  
    if (GET_CODE (*cur_rtx) == SUBREG
        && REG_P (p->x)
!       && (!REG_P (SUBREG_REG (*cur_rtx))
! 	  || REGNO (SUBREG_REG (*cur_rtx)) == REGNO (p->x)))
      {
        /* ??? Do not support substituting regs inside subregs.  In that case,
           simplify_subreg will be called by validate_replace_rtx, and

Re: [PATCH] Fix PR 44691

[Vladimir Makarov]

  On 08/18/2010 04:30 AM, Andrey Belevantsev wrote:
> Hello,
>
> As explained in the audit trail, the problem was that in the selective 
> scheduler I assumed that SUBREG_REG will always be a REG, which seems 
> to be not the case.  This is not quite in line with what documentation 
> says, if I read it correctly, but it seems to be used in a number of 
> backends, so the below patch just gives up substitution also when 
> SUBREG_REG is not a register.  Bootstrapped and tested on ia64, and 
> verified that the test is fixed on x86_64.
>
> I think that this qualifies as obvious, so unless Vlad or other people 
> have any comments, I'll commit it tomorrow.
>
Yes, it is obvious.
> Yours, Andrey
>
> 2010-08-18  Andrey Belevantsev <abel@ispras.ru>
>
>     PR rtl-optimization/44691
>
>     * sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
>     is not a register.
>
Ok to commit.  Thanks.

Re: [PATCH] Fix PR 44691

[H.J. Lu]

2010/8/18 Andrey Belevantsev <abel@ispras.ru>:
> Hello,
>
> As explained in the audit trail, the problem was that in the selective
> scheduler I assumed that SUBREG_REG will always be a REG, which seems to be
> not the case.  This is not quite in line with what documentation says, if I
> read it correctly, but it seems to be used in a number of backends, so the
> below patch just gives up substitution also when SUBREG_REG is not a
> register.  Bootstrapped and tested on ia64, and verified that the test is
> fixed on x86_64.
>
> I think that this qualifies as obvious, so unless Vlad or other people have
> any comments, I'll commit it tomorrow.
>
> Yours, Andrey
>
> 2010-08-18  Andrey Belevantsev  <abel@ispras.ru>
>
>        PR rtl-optimization/44691
>
>        * sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
>        is not a register.
>

Shouldn't we add the testcase?



-- 
H.J.

Re: [PATCH] Fix PR 44691

[Andrey Belevantsev]

On 19.08.2010 17:28, H.J. Lu wrote:
> 2010/8/18 Andrey Belevantsev<abel@ispras.ru>:
>> Hello,
>>
>> As explained in the audit trail, the problem was that in the selective
>> scheduler I assumed that SUBREG_REG will always be a REG, which seems to be
>> not the case.  This is not quite in line with what documentation says, if I
>> read it correctly, but it seems to be used in a number of backends, so the
>> below patch just gives up substitution also when SUBREG_REG is not a
>> register.  Bootstrapped and tested on ia64, and verified that the test is
>> fixed on x86_64.
>>
>> I think that this qualifies as obvious, so unless Vlad or other people have
>> any comments, I'll commit it tomorrow.
>>
>> Yours, Andrey
>>
>> 2010-08-18  Andrey Belevantsev<abel@ispras.ru>
>>
>>         PR rtl-optimization/44691
>>
>>         * sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
>>         is not a register.
>>
>
> Shouldn't we add the testcase?
The test is fortran.dg/pr42294.f which is actually mentioned in the bug 
report.  Sorry for not saying this explicitly in the mail.

Andrey

Re: [PATCH] Fix PR 44691

[H.J. Lu]

2010/8/19 Andrey Belevantsev <abel@ispras.ru>:
> On 19.08.2010 17:28, H.J. Lu wrote:
>>
>> 2010/8/18 Andrey Belevantsev<abel@ispras.ru>:
>>>
>>> Hello,
>>>
>>> As explained in the audit trail, the problem was that in the selective
>>> scheduler I assumed that SUBREG_REG will always be a REG, which seems to
>>> be
>>> not the case.  This is not quite in line with what documentation says, if
>>> I
>>> read it correctly, but it seems to be used in a number of backends, so
>>> the
>>> below patch just gives up substitution also when SUBREG_REG is not a
>>> register.  Bootstrapped and tested on ia64, and verified that the test is
>>> fixed on x86_64.
>>>
>>> I think that this qualifies as obvious, so unless Vlad or other people
>>> have
>>> any comments, I'll commit it tomorrow.
>>>
>>> Yours, Andrey
>>>
>>> 2010-08-18  Andrey Belevantsev<abel@ispras.ru>
>>>
>>>        PR rtl-optimization/44691
>>>
>>>        * sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
>>>        is not a register.
>>>
>>
>> Shouldn't we add the testcase?
>
> The test is fortran.dg/pr42294.f which is actually mentioned in the bug
> report.  Sorry for not saying this explicitly in the mail.
>

Normally this bug isn't trigged. You need to pass -O2 -fselective-scheduling2
to see it. You should copy gfortran.dg/pr42294.f and add  -O2
-fselective-scheduling2.



-- 
H.J.

Re: [PATCH] Fix PR 44691

[Andrey Belevantsev]

On 19.08.2010 18:12, H.J. Lu wrote:
> 2010/8/19 Andrey Belevantsev<abel@ispras.ru>:
>> On 19.08.2010 17:28, H.J. Lu wrote:
>>>
>>> 2010/8/18 Andrey Belevantsev<abel@ispras.ru>:
>>>>
>>>> Hello,
>>>>
>>>> As explained in the audit trail, the problem was that in the selective
>>>> scheduler I assumed that SUBREG_REG will always be a REG, which seems to
>>>> be
>>>> not the case.  This is not quite in line with what documentation says, if
>>>> I
>>>> read it correctly, but it seems to be used in a number of backends, so
>>>> the
>>>> below patch just gives up substitution also when SUBREG_REG is not a
>>>> register.  Bootstrapped and tested on ia64, and verified that the test is
>>>> fixed on x86_64.
>>>>
>>>> I think that this qualifies as obvious, so unless Vlad or other people
>>>> have
>>>> any comments, I'll commit it tomorrow.
>>>>
>>>> Yours, Andrey
>>>>
>>>> 2010-08-18  Andrey Belevantsev<abel@ispras.ru>
>>>>
>>>>         PR rtl-optimization/44691
>>>>
>>>>         * sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
>>>>         is not a register.
>>>>
>>>
>>> Shouldn't we add the testcase?
>>
>> The test is fortran.dg/pr42294.f which is actually mentioned in the bug
>> report.  Sorry for not saying this explicitly in the mail.
>>
>
> Normally this bug isn't trigged. You need to pass -O2 -fselective-scheduling2
> to see it. You should copy gfortran.dg/pr42294.f and add  -O2
> -fselective-scheduling2.
Ah, ok, I forgot about the explicit options.  I will do that.

Andrey

Re: [PATCH] Fix PR 44691

[Andrey Belevantsev]

On 19.08.2010 18:14, Andrey Belevantsev wrote:
> On 19.08.2010 18:12, H.J. Lu wrote:
>> 2010/8/19 Andrey Belevantsev<abel@ispras.ru>:
>>> On 19.08.2010 17:28, H.J. Lu wrote:
>>>>
>>>> 2010/8/18 Andrey Belevantsev<abel@ispras.ru>:
>>>>>
>>>>> Hello,
>>>>>
>>>>> As explained in the audit trail, the problem was that in the selective
>>>>> scheduler I assumed that SUBREG_REG will always be a REG, which seems to
>>>>> be
>>>>> not the case. This is not quite in line with what documentation says, if
>>>>> I
>>>>> read it correctly, but it seems to be used in a number of backends, so
>>>>> the
>>>>> below patch just gives up substitution also when SUBREG_REG is not a
>>>>> register. Bootstrapped and tested on ia64, and verified that the test is
>>>>> fixed on x86_64.
>>>>>
>>>>> I think that this qualifies as obvious, so unless Vlad or other people
>>>>> have
>>>>> any comments, I'll commit it tomorrow.
>>>>>
>>>>> Yours, Andrey
>>>>>
>>>>> 2010-08-18 Andrey Belevantsev<abel@ispras.ru>
>>>>>
>>>>> PR rtl-optimization/44691
>>>>>
>>>>> * sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
>>>>> is not a register.
>>>>>
>>>>
>>>> Shouldn't we add the testcase?
>>>
>>> The test is fortran.dg/pr42294.f which is actually mentioned in the bug
>>> report. Sorry for not saying this explicitly in the mail.
>>>
>>
>> Normally this bug isn't trigged. You need to pass -O2
>> -fselective-scheduling2
>> to see it. You should copy gfortran.dg/pr42294.f and add -O2
>> -fselective-scheduling2.
> Ah, ok, I forgot about the explicit options. I will do that.
Looking closely, pr42294.f happens to be another sel-sched bug, so it 
already has "-O2 -fselective-scheduling2 -fsel-sched-pipelining 
-funroll-all-loops" as dg-options.  So I guess this test should be enough, 
what do you think?

Andrey

Re: [PATCH] Fix PR 44691

[H.J. Lu]

2010/8/19 Andrey Belevantsev <abel@ispras.ru>:
> On 19.08.2010 18:14, Andrey Belevantsev wrote:
>>
>> On 19.08.2010 18:12, H.J. Lu wrote:
>>>
>>> 2010/8/19 Andrey Belevantsev<abel@ispras.ru>:
>>>>
>>>> On 19.08.2010 17:28, H.J. Lu wrote:
>>>>>
>>>>> 2010/8/18 Andrey Belevantsev<abel@ispras.ru>:
>>>>>>
>>>>>> Hello,
>>>>>>
>>>>>> As explained in the audit trail, the problem was that in the selective
>>>>>> scheduler I assumed that SUBREG_REG will always be a REG, which seems
>>>>>> to
>>>>>> be
>>>>>> not the case. This is not quite in line with what documentation says,
>>>>>> if
>>>>>> I
>>>>>> read it correctly, but it seems to be used in a number of backends, so
>>>>>> the
>>>>>> below patch just gives up substitution also when SUBREG_REG is not a
>>>>>> register. Bootstrapped and tested on ia64, and verified that the test
>>>>>> is
>>>>>> fixed on x86_64.
>>>>>>
>>>>>> I think that this qualifies as obvious, so unless Vlad or other people
>>>>>> have
>>>>>> any comments, I'll commit it tomorrow.
>>>>>>
>>>>>> Yours, Andrey
>>>>>>
>>>>>> 2010-08-18 Andrey Belevantsev<abel@ispras.ru>
>>>>>>
>>>>>> PR rtl-optimization/44691
>>>>>>
>>>>>> * sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
>>>>>> is not a register.
>>>>>>
>>>>>
>>>>> Shouldn't we add the testcase?
>>>>
>>>> The test is fortran.dg/pr42294.f which is actually mentioned in the bug
>>>> report. Sorry for not saying this explicitly in the mail.
>>>>
>>>
>>> Normally this bug isn't trigged. You need to pass -O2
>>> -fselective-scheduling2
>>> to see it. You should copy gfortran.dg/pr42294.f and add -O2
>>> -fselective-scheduling2.
>>
>> Ah, ok, I forgot about the explicit options. I will do that.
>
> Looking closely, pr42294.f happens to be another sel-sched bug, so it
> already has "-O2 -fselective-scheduling2 -fsel-sched-pipelining
> -funroll-all-loops" as dg-options.  So I guess this test should be enough,
> what do you think?

Why didn't it fail before? Does this bug fail only with "-O2
-fselective-scheduling2"?


-- 
H.J.

Re: [PATCH] Fix PR 44691

[Andrey Belevantsev]

On 19.08.2010 19:53, H.J. Lu wrote:
> 2010/8/19 Andrey Belevantsev<abel@ispras.ru>:
>> On 19.08.2010 18:14, Andrey Belevantsev wrote:
>>>
>>> On 19.08.2010 18:12, H.J. Lu wrote:
>>>>
>>>> 2010/8/19 Andrey Belevantsev<abel@ispras.ru>:
>>>>>
>>>>> On 19.08.2010 17:28, H.J. Lu wrote:
>>>>>>
>>>>>> 2010/8/18 Andrey Belevantsev<abel@ispras.ru>:
>>>>>>>
>>>>>>> Hello,
>>>>>>>
>>>>>>> As explained in the audit trail, the problem was that in the selective
>>>>>>> scheduler I assumed that SUBREG_REG will always be a REG, which seems
>>>>>>> to
>>>>>>> be
>>>>>>> not the case. This is not quite in line with what documentation says,
>>>>>>> if
>>>>>>> I
>>>>>>> read it correctly, but it seems to be used in a number of backends, so
>>>>>>> the
>>>>>>> below patch just gives up substitution also when SUBREG_REG is not a
>>>>>>> register. Bootstrapped and tested on ia64, and verified that the test
>>>>>>> is
>>>>>>> fixed on x86_64.
>>>>>>>
>>>>>>> I think that this qualifies as obvious, so unless Vlad or other people
>>>>>>> have
>>>>>>> any comments, I'll commit it tomorrow.
>>>>>>>
>>>>>>> Yours, Andrey
>>>>>>>
>>>>>>> 2010-08-18 Andrey Belevantsev<abel@ispras.ru>
>>>>>>>
>>>>>>> PR rtl-optimization/44691
>>>>>>>
>>>>>>> * sel-sched.c (count_occurrences_1): Also punt when SUBREG_REG
>>>>>>> is not a register.
>>>>>>>
>>>>>>
>>>>>> Shouldn't we add the testcase?
>>>>>
>>>>> The test is fortran.dg/pr42294.f which is actually mentioned in the bug
>>>>> report. Sorry for not saying this explicitly in the mail.
>>>>>
>>>>
>>>> Normally this bug isn't trigged. You need to pass -O2
>>>> -fselective-scheduling2
>>>> to see it. You should copy gfortran.dg/pr42294.f and add -O2
>>>> -fselective-scheduling2.
>>>
>>> Ah, ok, I forgot about the explicit options. I will do that.
>>
>> Looking closely, pr42294.f happens to be another sel-sched bug, so it
>> already has "-O2 -fselective-scheduling2 -fsel-sched-pipelining
>> -funroll-all-loops" as dg-options.  So I guess this test should be enough,
>> what do you think?
>
> Why didn't it fail before? Does this bug fail only with "-O2
> -fselective-scheduling2"?
As mentioned in the PR, it started to fail with addition of the lea splits. 
  But you are right, you need to remove -funroll-loops from the compile 
options for the test to fail, so I have added a copy with just -O2 
-fselective-scheduling2 and committed as 163396.

Thanks for noticing,
Andrey