Re: [PATCH] PR target/70155: Use SSE for TImode load/store

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2016-04-26 19:12 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
> On Tue, Apr 26, 2016 at 9:07 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> 2016-04-26 18:39 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>> On Tue, Apr 26, 2016 at 8:21 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>> 2016-04-26 18:12 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>> On Tue, Apr 26, 2016 at 8:05 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>> 2016-04-26 17:55 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>> On Tue, Apr 26, 2016 at 7:15 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>>>> 2016-04-26 17:07 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>>>> On Mon, Apr 25, 2016 at 9:13 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>>>>>>>>>> 2016-04-25 18:27 GMT+03:00 H.J. Lu <hjl.tools@gmail.com>:
>>>>>>>>>>>
>>>>>>>>>>> Ilya, can you take a look?
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>> --
>>>>>>>>>>> H.J.
>>>>>>>>>>
>>>>>>>>>> Hi,
>>>>>>>>>>
>>>>>>>>>> Algorithmic part of the patch looks OK to me except the following piece of code.
>>>>>>>>>>
>>>>>>>>>> +/* Check REF's chain to add new insns into a queue
>>>>>>>>>> +   and find registers requiring conversion.  */
>>>>>>>>>>
>>>>>>>>>> Comment is wrong because you don't have any conversions required for
>>>>>>>>>> your candidates.
>>>>>>>>>
>>>>>>>>> I will fix it.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +void
>>>>>>>>>> +scalar_chain_64::analyze_register_chain (bitmap candidates, df_ref ref)
>>>>>>>>>> +{
>>>>>>>>>> +  df_link *chain;
>>>>>>>>>> +
>>>>>>>>>> +  gcc_assert (bitmap_bit_p (insns, DF_REF_INSN_UID (ref))
>>>>>>>>>> +             || bitmap_bit_p (candidates, DF_REF_INSN_UID (ref)));
>>>>>>>>>> +  add_to_queue (DF_REF_INSN_UID (ref));
>>>>>>>>>> +
>>>>>>>>>> +  for (chain = DF_REF_CHAIN (ref); chain; chain = chain->next)
>>>>>>>>>> +    {
>>>>>>>>>> +      unsigned uid = DF_REF_INSN_UID (chain->ref);
>>>>>>>>>> +
>>>>>>>>>> +      if (!NONDEBUG_INSN_P (DF_REF_INSN (chain->ref)))
>>>>>>>>>> +       continue;
>>>>>>>>>> +
>>>>>>>>>> +      if (!DF_REF_REG_MEM_P (chain->ref))
>>>>>>>>>> +       continue;
>>>>>>>>>>
>>>>>>>>>> I believe here you wrongly jump to the next ref intead of actually adding it
>>>>>>>>>> to a queue.  You may just use
>>>>>>>>>>
>>>>>>>>>> gcc_assert (!DF_REF_REG_MEM_P (chain->ref));
>>>>>>>>>>
>>>>>>>>>> because you should'n have a candidate used in address operand.
>>>>>>>>>
>>>>>>>>> I will update.
>>>>>>>>>
>>>>>>>>>> +
>>>>>>>>>> +      if (bitmap_bit_p (insns, uid))
>>>>>>>>>> +       continue;
>>>>>>>>>> +
>>>>>>>>>> +      if (bitmap_bit_p (candidates, uid))
>>>>>>>>>> +       add_to_queue (uid);
>>>>>>>>>>
>>>>>>>>>> Probably gcc_assert (bitmap_bit_p (candidates, uid)) since no uses and defs
>>>>>>>>>> out of candidates list are allowed?
>>>>>>>>>
>>>>>>>>> That would be wrong since there are
>>>>>>>>>
>>>>>>>>>  while (!bitmap_empty_p (queue))
>>>>>>>>>     {
>>>>>>>>>       insn_uid = bitmap_first_set_bit (queue);
>>>>>>>>>       bitmap_clear_bit (queue, insn_uid);
>>>>>>>>>       bitmap_clear_bit (candidates, insn_uid);
>>>>>>>>>       add_insn (candidates, insn_uid);
>>>>>>>>>     }
>>>>>>>>>
>>>>>>>>> An instruction is a candidate and the bit is cleared when
>>>>>>>>> analyze_register_chain is called.
>>>>>>>>
>>>>>>>> You clear candidates bit but the first thing you do in add_insn is set
>>>>>>>> insns bit.
>>>>>>>> Thus you should hit:
>>>>>>>>
>>>>>>>>       if (bitmap_bit_p (insns, uid))
>>>>>>>>         continue;
>>>>>>>>
>>>>>>>> For handled candidates.
>>>>>>>>
>>>>>>>> Probably it would be more clear if we keep this clear/set pair
>>>>>>>> together?  E.g. move
>>>>>>>> bitmap_clear_bit (candidates, insn_uid) to scalar_chain::add_insn.
>>>>>>>>
>>>>>>>
>>>>>>> After we started processing candidates, we only use candidates
>>>>>>> to check if an instruction is a candidate, not to check if an
>>>>>>> instruction is NOT a candidate.
>>>>>>
>>>>>> I don't see how it's related to what I said.  My point is that
>>>>>> when you analyze added insn you shouldn't reach insns which are both
>>>>>> not in candidates and not in current scalar_chain_64.  That's why I
>>>>>> think you miss an assert in scalar_chain_64::analyze_register_chain.
>>>>>
>>>>> Since all candidates will be processed by
>>>>>
>>>>>  while (!bitmap_empty_p (queue))
>>>>>     {
>>>>>       insn_uid = bitmap_first_set_bit (queue);
>>>>>       bitmap_clear_bit (queue, insn_uid);
>>>>>       bitmap_clear_bit (candidates, insn_uid);
>>>>>       add_insn (candidates, insn_uid);
>>>>>     }
>>>>
>>>> This process only queue, not all candidates.  analyze_register_chain
>>>> fills queue bitmap to build a chain.
>>>>
>>>>>
>>>>> I will change to
>>>>>
>>>>> /* Check REF's chain to add new insns into a queue.  */
>>>>>
>>>>> void
>>>>> timode_scalar_chain::analyze_register_chain (bitmap candidates,
>>>>>                                              df_ref ref)
>>>>> {
>>>>>     gcc_assert (bitmap_bit_p (insns, DF_REF_INSN_UID (ref))
>>>>>               || bitmap_bit_p (candidates, DF_REF_INSN_UID (ref)));
>>>>>   add_to_queue (DF_REF_INSN_UID (ref));
>>>>> }
>>>>>
>>>>
>>>> The purpose of analyze_register_chain is to collect all register defs
>>>> or uses into chain's queue.  You can't just remove a loop over ref's
>>>> chain then.
>>>
>>> That is true for DImode conversion.  For TImode conversion,
>>> there are no register conversions required:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01394.html
>>>
>>> All it does it to call add_to_queue.
>>
>> No conversion means no mark_dual_mode_def calls in analyze_register_chain
>> but it still may call add_to_queue multiple times in its loop. E.g.
>>
>> 1: r1 = 1
>> ...
>> 5: r1 = 2
>> ..
>> 10: r2 = r1
>>
>> When you add #10 into a chain you should add both #1 and #5 into a queue.
>> That's what analyze_register_chain is supposed to do.
>
> Since both i1 and i5 are candidates, they are added to queue.

By who?  They will just go into another chain.

Thanks,
Ilya

>
> --
> H.J.