2016-04-26 17:55 GMT+03:00 H.J. Lu <hjl.to...@gmail.com>:
> On Tue, Apr 26, 2016 at 7:15 AM, Ilya Enkovich <enkovich....@gmail.com> wrote:
>> 2016-04-26 17:07 GMT+03:00 H.J. Lu <hjl.to...@gmail.com>:
>>> On Mon, Apr 25, 2016 at 9:13 AM, Ilya Enkovich <enkovich....@gmail.com>
>>> wrote:
>>>> 2016-04-25 18:27 GMT+03:00 H.J. Lu <hjl.to...@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.
Thanks,
Ilya
>
> --
> H.J.
