Re: Shouldn't convert_scalars_to_vector call free_dominance_info?

[Richard Biener <richard dot guenther at gmail dot com>]

On Fri, Mar 11, 2016 at 2:02 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Fri, Mar 11, 2016 at 4:58 AM, Richard Biener
> <richard.guenther@gmail.com> wrote:
>> On Fri, Mar 11, 2016 at 1:47 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Fri, Mar 11, 2016 at 2:06 AM, Richard Biener
>>> <richard.guenther@gmail.com> wrote:
>>>> On Fri, Mar 11, 2016 at 4:10 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Thu, Mar 10, 2016 at 7:03 PM, Jeff Law <law@redhat.com> wrote:
>>>>>> On 03/10/2016 08:00 PM, H.J. Lu wrote:
>>>>>>>
>>>>>>> On Thu, Mar 10, 2016 at 1:30 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On Thu, Mar 10, 2016 at 1:24 PM, Jeff Law <law@redhat.com> wrote:
>>>>>>>>>
>>>>>>>>> On 03/10/2016 01:18 PM, Richard Biener wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On March 10, 2016 6:02:58 PM GMT+01:00, "H.J. Lu" <hjl.tools@gmail.com>
>>>>>>>>>> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On Thu, Mar 10, 2016 at 6:57 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On Thu, Mar 10, 2016 at 5:49 AM, Jakub Jelinek <jakub@redhat.com>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> wrote:
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> On Thu, Mar 10, 2016 at 05:43:27AM -0800, H.J. Lu wrote:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> free_dominance_info (CDI_DOMINATORS);
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Since convert_scalars_to_vector may add instructions, dominance
>>>>>>>>>>>>>> info is no longer up to date.
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Adding instructions doesn't change anything on the dominance info,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> just
>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> cfg manipulations that don't keep the dominators updated.
>>>>>>>>>>>>> You can try to verify the dominance info at the end of the stv pass,
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> I added
>>>>>>>>>>>>
>>>>>>>>>>>> verify_dominators (CDI_DOMINATORS);
>>>>>>>>>>>> '
>>>>>>>>>>>> It did trigger assert in my 64-bit STV pass in 64-bit libgcc build:
>>>>>>>>>>>>
>>>>>>>>>>>> /export/gnu/import/git/sources/gcc/libgcc/config/libbid/bid128_fma.c:
>>>>>>>>>>>> In function \u2018add_and_round.constprop\u2019:
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> /export/gnu/import/git/sources/gcc/libgcc/config/libbid/bid128_fma.c:629:1:
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> error: dominator of 158 should be 107, not 101
>>>>>>>>>>>>
>>>>>>>>>>>> I will investigate.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Here is the problem:
>>>>>>>>>>>
>>>>>>>>>>> 1. I extended the STV pass to 64-bit to convert TI load/store to
>>>>>>>>>>> V1TI load/store to use SSE load/store for 128-bit load/store.
>>>>>>>>>>> 2. The 64-bit STV pass generates settings of CONST0_RTX and
>>>>>>>>>>> CONSTM1_RTX to store 128-bit 0 and -1.
>>>>>>>>>>> 3. I placed the 64-bit STV pass before the CSE pass so that
>>>>>>>>>>> CONST0_RTX and CONSTM1_RTX generated by the STV pass
>>>>>>>>>>> can be CSEed.
>>>>>>>>>>> 4. After settings of CONST0_RTX and CONSTM1_RTX are CSEed,
>>>>>>>>>>> dominance info will be wrong.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Can't see how cse can ever invalidate dominators.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> cse can simplify jumps which can invalidate dominance information.
>>>>>>>>>
>>>>>>>>> But cse-ing CONST0_RTX and CONSTM1_RTX shouldn't invalidate dominators,
>>>>>>>>> that's just utter nonsense -- ultimately it has to come down to changing
>>>>>>>>> jumps.  ISTM HJ has more digging to do here.
>>>>>>>>
>>>>>>>>
>>>>>>>> Not just CONST0_RTX and CONSTM1_RTX.  The new STV
>>>>>>>> pass changes mode of SET from TImode to V1TImode which
>>>>>>>> exposes more opportunities to CSE.
>>>>>>>>
>>>>>>>
>>>>>>> What I did is equivalent to
>>>>>>>
>>>>>>> diff --git a/gcc/cse.c b/gcc/cse.c
>>>>>>> index 2665d9a..43202a1 100644
>>>>>>> --- a/gcc/cse.c
>>>>>>> +++ b/gcc/cse.c
>>>>>>> @@ -7644,7 +7644,11 @@ public:
>>>>>>>         return optimize > 0 && flag_rerun_cse_after_loop;
>>>>>>>       }
>>>>>>>
>>>>>>> -  virtual unsigned int execute (function *) { return rest_of_handle_cse2
>>>>>>> (); }
>>>>>>> +  virtual unsigned int execute (function *)
>>>>>>> +    {
>>>>>>> +      calculate_dominance_info (CDI_DOMINATORS);
>>>>>>> +      return rest_of_handle_cse2 ();
>>>>>>> +    }
>>>>>>>
>>>>>>>   }; // class pass_cse2
>>>>>>>
>>>>>>>
>>>>>>> which leads to the same ICE:
>>>>>>
>>>>>> But you haven't done the proper analysis to understand why the dominance
>>>>>> relationships have changed.  Nothing of the changes you've outlined in your
>>>>>> messages should invalidate the dominance information.
>>>>>
>>>>> Nothing is changed.  Just calling
>>>>>
>>>>> calculate_dominance_info (CDI_DOMINATORS);
>>>>>
>>>>> before rest_of_handle_cse2 will lead to ICE.
>>>>
>>>> Well, so CSE2 invalidates dominators but fails to free them when necessary.
>>>> Please figure out the CSE transform that invalidates them and free dominators
>>>> there.
>>>
>>> I can give it a try.  But I'd like to first ask since CSE2 never calls
>>> calculate_dominance_info (CDI_DOMINATORS), does it need to
>>> keep dominators valid?
>>
>> If it doesn't free them then yes.
>>
>>> free_dominance_info (CDI_DOMINATORS);
>>>
>>> at beginning will do the job.
>>
>> Of course.  But that may be not always necessary and thus cause extra
>> dominance compute for the next user.
>
> Do we need to both CDI_DOMINATORS and CDI_POST_DOMINATORS
> valid?

CDI_POST_DOMINATORS is required to be freed by passes.

Richard.

>
> --
> H.J.