2016-04-26 17:07 GMT+03:00 H.J. Lu <[email protected]>:
> On Mon, Apr 25, 2016 at 9:13 AM, Ilya Enkovich <[email protected]> wrote:
>> 2016-04-25 18:27 GMT+03:00 H.J. Lu <[email protected]>:
>>>
>>> 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.
Thanks,
Ilya
>
> --
> H.J.
