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

Richard.

> --
> H.J.