Re: Shouldn't convert_scalars_to_vector call free_dominance_info?

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.

-- 
H.J.