On Mon, Oct 19, 2020 at 11:38 PM Vladimir Makarov <vmaka...@redhat.com> wrote:
>
>
> On 2020-10-11 8:58 p.m., Hongtao Liu wrote:
> > Hi:
> > This is done in 2 steps:
> > 1. Extend special memory constraint to handle non MEM_P cases, i.e.
> > (vec_duplicate:V4SF (mem:SF (addr)))
> > 2. Refactor implementation of *_bcst{_1,_2,_3} patterns. Add new
> > predicate bcst_mem_operand and corresponding constraint "Br" to merge
> > "$(pattern)_bcst{_1,_2,_3}" into "$(pattern)", also delete those
> > separate "*_bcst{_1,_2,_3}" patterns.
> >
> > Bootstrap is ok, regression test on i386 backend is ok.
> >
> > gcc/ChangeLog:
> >
> > PR target/87767
> > * ira-costs.c (record_operand_costs): Extract memory operand
> > from recog_data.operand[i] for record_address_regs.
> > (record_reg_classes): Extract memory operand from OP for
> > conditional judgement MEM_P.
> > * ira.c (ira_setup_alts): Ditto.
> > * lra-constraints.c (extract_mem_from_operand): New function.
> > (satisfies_memory_constraint_p): Extract memory operand from
> > OP for decompose_mem_address, return false when there's no
> > memory operand inside OP.
> > (process_alt_operands): Remove MEM_P (op) since it would be
> > judged in satisfies_memory_constraint_p.
> > * recog.c (asm_operand_ok): Extract memory operand from OP for
> > judgement of memory_operand (OP, VOIDmode).
> > (constrain_operands): Don't unwrapper unary operator when
> > there's memory operand inside.
> > * rtl.h (extract_mem_from_operand): New decl.
>
>
> Thank you for working on the PR. In general patch is ok for me. The
> only thing is
>
> +/* For special_memory_operand, it could be false for MEM_P (op),
> + i.e. bcst_mem_operand in i386 backend.
> + Extract and return real memory operand or op. */
> +rtx
> +extract_mem_from_operand (rtx op)
> +{
> + if (MEM_P (op))
> + return op;
> + /* Only allow one memory_operand inside special memory operand. */
>
> The comment contradicts to the below code which returns the first memory
> operand (not the only one).
>
Yes.
> + subrtx_var_iterator::array_type array;
> + FOR_EACH_SUBRTX_VAR (iter, array, op, ALL)
> + {
> + rtx x = *iter;
> + if (MEM_P (x))
> + return x;
> + }
> +
> + return op;
> +}
> +
>
> I think the code should look like
>
> /* For special_memory_operand, it could be false for MEM_P (op),
> i.e. bcst_mem_operand in i386 backend.
> Extract and return real memory operand or op. */
> rtx
> extract_mem_from_operand (rtx op)
> {
> if (MEM_P (op))
> return op;
> /* Only allow one memory_operand inside special memory operand. */
> subrtx_var_iterator::array_type array;
> rtx res = op;
> FOR_EACH_SUBRTX_VAR (iter, array, op, ALL)
> {
> rtx x = *iter;
> if (!MEM_P (x) || res != op)
> return op;
> res = op;
Assume you want to assign res with x.
Also in the iteration, x would first be op which would be false for
MEM_P, then op would be returned.
That's not what you mean, so i changed to
/* Only allow one memory_operand inside special memory operand. */
subrtx_var_iterator::array_type array;
rtx res = op;
FOR_EACH_SUBRTX_VAR (iter, array, op, ALL)
{
rtx x = *iter;
if (!MEM_P (x))
continue;
/* Return op when there're multiple memory operands. */
if (res != op)
return op;
else
res = x;
}
> }
>
> return res;
> }
>
>
> With this change, the patch is ok for me and can be committed into the trunk.
>
--
BR,
Hongtao
