[2/7] Do not query SCEV in range_of_phi unless dominators are available.

Message ID 20210921165350.414593-3-aldyh@redhat.com
State New
Headers
Series Add ability to resolve unknowns to path solver. |

Commit Message

Aldy Hernandez Sept. 21, 2021, 4:53 p.m. UTC
  SCEV won't work without dominators and we can get called without
dominators from debug_ranger.

Another option would be to rename scev_initialized_p to something like
scev_available_p and move the check there.  For now, this will do.

Committed.

gcc/ChangeLog:

	* gimple-range-fold.cc (fold_using_range::range_of_phi): Check
	dom_info_available_p.
---
 gcc/gimple-range-fold.cc | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
  

Comments

Andrew MacLeod Sept. 21, 2021, 5:05 p.m. UTC | #1
On 9/21/21 12:53 PM, Aldy Hernandez wrote:
> SCEV won't work without dominators and we can get called without
> dominators from debug_ranger.
>
> Another option would be to rename scev_initialized_p to something like
> scev_available_p and move the check there.  For now, this will do.
>
> Committed.
>
> gcc/ChangeLog:
>
> 	* gimple-range-fold.cc (fold_using_range::range_of_phi): Check
> 	dom_info_available_p.
> ---
>   gcc/gimple-range-fold.cc | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
> index 997d02dd4b9..4dbf4188ec2 100644
> --- a/gcc/gimple-range-fold.cc
> +++ b/gcc/gimple-range-fold.cc
> @@ -781,7 +781,9 @@ fold_using_range::range_of_phi (irange &r, gphi *phi, fur_source &src)
>       }
>   
>     // If SCEV is available, query if this PHI has any knonwn values.
> -  if (scev_initialized_p () && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
> +  if (dom_info_available_p (CDI_DOMINATORS)
> +      && scev_initialized_p ()
> +      && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
>       {
>         value_range loop_range;
>         class loop *l = loop_containing_stmt (phi);

Im confused.. if scev doesn't work without dominators, how is 
scev_initialized_p() true if there are no dominators?     Are we 
initializing it somewhere without dominators?  Maybe there should be a 
check in the scev init routine?  seems like something else is amok.


void
scev_initialize (void)
{
   gcc_assert (! scev_initialized_p ());

   scalar_evolution_info = hash_table<scev_info_hasher>::create_ggc (100);

   for (auto loop : loops_list (cfun, 0))
     loop->nb_iterations = NULL_TREE;
}

/* Return true if SCEV is initialized.  */

bool
scev_initialized_p (void)
{
   return scalar_evolution_info != NULL;
}
  
Aldy Hernandez Sept. 21, 2021, 5:16 p.m. UTC | #2
On 9/21/21 7:05 PM, Andrew MacLeod wrote:
> On 9/21/21 12:53 PM, Aldy Hernandez wrote:
>> SCEV won't work without dominators and we can get called without
>> dominators from debug_ranger.
>>
>> Another option would be to rename scev_initialized_p to something like
>> scev_available_p and move the check there.  For now, this will do.
>>
>> Committed.
>>
>> gcc/ChangeLog:
>>
>>     * gimple-range-fold.cc (fold_using_range::range_of_phi): Check
>>     dom_info_available_p.
>> ---
>>   gcc/gimple-range-fold.cc | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
>> index 997d02dd4b9..4dbf4188ec2 100644
>> --- a/gcc/gimple-range-fold.cc
>> +++ b/gcc/gimple-range-fold.cc
>> @@ -781,7 +781,9 @@ fold_using_range::range_of_phi (irange &r, gphi 
>> *phi, fur_source &src)
>>       }
>>     // If SCEV is available, query if this PHI has any knonwn values.
>> -  if (scev_initialized_p () && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
>> +  if (dom_info_available_p (CDI_DOMINATORS)
>> +      && scev_initialized_p ()
>> +      && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
>>       {
>>         value_range loop_range;
>>         class loop *l = loop_containing_stmt (phi);
> 
> Im confused.. if scev doesn't work without dominators, how is 
> scev_initialized_p() true if there are no dominators?     Are we 
> initializing it somewhere without dominators?  Maybe there should be a 
> check in the scev init routine?  seems like something else is amok.

As I mentioned, this can happen from debug_ranger(), which is a 
debugging construct, and I've been known to call it without dominators 
:).  And yes, I agree we could move it to scev_initialized_p.

Aldy

> 
> 
> void
> scev_initialize (void)
> {
>    gcc_assert (! scev_initialized_p ());
> 
>    scalar_evolution_info = hash_table<scev_info_hasher>::create_ggc (100);
> 
>    for (auto loop : loops_list (cfun, 0))
>      loop->nb_iterations = NULL_TREE;
> }
> 
> /* Return true if SCEV is initialized.  */
> 
> bool
> scev_initialized_p (void)
> {
>    return scalar_evolution_info != NULL;
> }
> 
>
  
Richard Biener Sept. 22, 2021, 8:05 a.m. UTC | #3
On Tue, Sep 21, 2021 at 7:17 PM Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
>
>
> On 9/21/21 7:05 PM, Andrew MacLeod wrote:
> > On 9/21/21 12:53 PM, Aldy Hernandez wrote:
> >> SCEV won't work without dominators and we can get called without
> >> dominators from debug_ranger.
> >>
> >> Another option would be to rename scev_initialized_p to something like
> >> scev_available_p and move the check there.  For now, this will do.
> >>
> >> Committed.
> >>
> >> gcc/ChangeLog:
> >>
> >>     * gimple-range-fold.cc (fold_using_range::range_of_phi): Check
> >>     dom_info_available_p.
> >> ---
> >>   gcc/gimple-range-fold.cc | 4 +++-
> >>   1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
> >> index 997d02dd4b9..4dbf4188ec2 100644
> >> --- a/gcc/gimple-range-fold.cc
> >> +++ b/gcc/gimple-range-fold.cc
> >> @@ -781,7 +781,9 @@ fold_using_range::range_of_phi (irange &r, gphi
> >> *phi, fur_source &src)
> >>       }
> >>     // If SCEV is available, query if this PHI has any knonwn values.
> >> -  if (scev_initialized_p () && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
> >> +  if (dom_info_available_p (CDI_DOMINATORS)
> >> +      && scev_initialized_p ()
> >> +      && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
> >>       {
> >>         value_range loop_range;
> >>         class loop *l = loop_containing_stmt (phi);
> >
> > Im confused.. if scev doesn't work without dominators, how is
> > scev_initialized_p() true if there are no dominators?     Are we
> > initializing it somewhere without dominators?  Maybe there should be a
> > check in the scev init routine?  seems like something else is amok.
>
> As I mentioned, this can happen from debug_ranger(), which is a
> debugging construct, and I've been known to call it without dominators
> :).  And yes, I agree we could move it to scev_initialized_p.

But why is scev_initialized_p () true from debug_ranger()?

> Aldy
>
> >
> >
> > void
> > scev_initialize (void)
> > {
> >    gcc_assert (! scev_initialized_p ());
> >
> >    scalar_evolution_info = hash_table<scev_info_hasher>::create_ggc (100);
> >
> >    for (auto loop : loops_list (cfun, 0))
> >      loop->nb_iterations = NULL_TREE;
> > }
> >
> > /* Return true if SCEV is initialized.  */
> >
> > bool
> > scev_initialized_p (void)
> > {
> >    return scalar_evolution_info != NULL;
> > }
> >
> >
>
  
Aldy Hernandez Sept. 23, 2021, 11:07 a.m. UTC | #4
On 9/22/21 10:05 AM, Richard Biener wrote:
> On Tue, Sep 21, 2021 at 7:17 PM Aldy Hernandez via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>>
>>
>> On 9/21/21 7:05 PM, Andrew MacLeod wrote:
>>> On 9/21/21 12:53 PM, Aldy Hernandez wrote:
>>>> SCEV won't work without dominators and we can get called without
>>>> dominators from debug_ranger.
>>>>
>>>> Another option would be to rename scev_initialized_p to something like
>>>> scev_available_p and move the check there.  For now, this will do.
>>>>
>>>> Committed.
>>>>
>>>> gcc/ChangeLog:
>>>>
>>>>      * gimple-range-fold.cc (fold_using_range::range_of_phi): Check
>>>>      dom_info_available_p.
>>>> ---
>>>>    gcc/gimple-range-fold.cc | 4 +++-
>>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
>>>> index 997d02dd4b9..4dbf4188ec2 100644
>>>> --- a/gcc/gimple-range-fold.cc
>>>> +++ b/gcc/gimple-range-fold.cc
>>>> @@ -781,7 +781,9 @@ fold_using_range::range_of_phi (irange &r, gphi
>>>> *phi, fur_source &src)
>>>>        }
>>>>      // If SCEV is available, query if this PHI has any knonwn values.
>>>> -  if (scev_initialized_p () && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
>>>> +  if (dom_info_available_p (CDI_DOMINATORS)
>>>> +      && scev_initialized_p ()
>>>> +      && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
>>>>        {
>>>>          value_range loop_range;
>>>>          class loop *l = loop_containing_stmt (phi);
>>>
>>> Im confused.. if scev doesn't work without dominators, how is
>>> scev_initialized_p() true if there are no dominators?     Are we
>>> initializing it somewhere without dominators?  Maybe there should be a
>>> check in the scev init routine?  seems like something else is amok.
>>
>> As I mentioned, this can happen from debug_ranger(), which is a
>> debugging construct, and I've been known to call it without dominators
>> :).  And yes, I agree we could move it to scev_initialized_p.
> 
> But why is scev_initialized_p () true from debug_ranger()?

This seems to be an artifact of some internal diagnostic code I had 
comparing the new work with the ASSERT_EXPR threads.  I've reverted the 
patch.

Sorry for the noise, and thanks for pointing it out.
Aldy
  

Patch

diff --git a/gcc/gimple-range-fold.cc b/gcc/gimple-range-fold.cc
index 997d02dd4b9..4dbf4188ec2 100644
--- a/gcc/gimple-range-fold.cc
+++ b/gcc/gimple-range-fold.cc
@@ -781,7 +781,9 @@  fold_using_range::range_of_phi (irange &r, gphi *phi, fur_source &src)
     }
 
   // If SCEV is available, query if this PHI has any knonwn values.
-  if (scev_initialized_p () && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
+  if (dom_info_available_p (CDI_DOMINATORS)
+      && scev_initialized_p ()
+      && !POINTER_TYPE_P (TREE_TYPE (phi_def)))
     {
       value_range loop_range;
       class loop *l = loop_containing_stmt (phi);