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