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).

+  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;
    }

  return res;
}


With this change, the patch is ok for me and can be committed into the trunk.

Reply via email to