On Tue, Oct 20, 2020 at 10:57 PM Vladimir Makarov <vmaka...@redhat.com> wrote:
>
>
> On 2020-10-20 1:33 a.m., Hongtao Liu wrote:
> > 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;
> > }
>
> Actually I wanted to have constraint satisfying rtx with memory covered
> by **only unary** operator(s). Your code satisfies memory covered by
> non-unary operators (e.g. binary ones).
>
> Why do I prefer less general constraint? Because other operands of
> operator containing the memory might need reloads too and the more
> general constraint will ignore this. If this situation is impossible
> now, it might be possible in the future.
>
Got your point.
> My proposed code is wrong as I forgot that FOR_EACH_SUBRTX_VAR processes
> sub-rtx recursively. Thank you for starting the discussion. Now 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)
> {
> for (rtx x = op;; x = XEXP (x, 0)) {
>
> if (MEM_P (x))
> return x;
> if (GET_RTX_LENGTH (GET_CODE (x)) != 1 || GET_RTX_FORMAT (GET_CODE
> (x))[0] != 'e')
> break;
>
> }
>
> return op;
>
> }
>
> Let me know what do you think.
>
Changed, and it passed the i386/x86-64 regression test.
Update patch.
--
BR,
Hongtao
From 65e7fa0807d31a17d6772f4292176ad4ecbb571d Mon Sep 17 00:00:00 2001
From: liuhongt <hongtao....@intel.com>
Date: Sat, 26 Sep 2020 15:08:32 +0800
Subject: [PATCH 1/2] Extend special_memory_constraint.
For operand with special_memory_constraint, there could be a wrapper
for memory_operand. Extract mem for operand for conditional judgement
like MEM_P, also for record_address_regs.
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.
---
gcc/ira-costs.c | 12 +++++++-----
gcc/ira.c | 2 +-
gcc/lra-constraints.c | 28 +++++++++++++++++++++++-----
gcc/recog.c | 7 +++++--
gcc/rtl.h | 1 +
5 files changed, 37 insertions(+), 13 deletions(-)
diff --git a/gcc/ira-costs.c b/gcc/ira-costs.c
index 6891156b5aa..aeda6588bcd 100644
--- a/gcc/ira-costs.c
+++ b/gcc/ira-costs.c
@@ -781,7 +781,8 @@ record_reg_classes (int n_alts, int n_ops, rtx *ops,
case CT_SPECIAL_MEMORY:
insn_allows_mem[i] = allows_mem[i] = 1;
- if (MEM_P (op) && constraint_satisfied_p (op, cn))
+ if (MEM_P (extract_mem_from_operand (op))
+ && constraint_satisfied_p (op, cn))
win = 1;
break;
@@ -1397,15 +1398,16 @@ record_operand_costs (rtx_insn *insn, enum reg_class *pref)
commutative. */
for (i = 0; i < recog_data.n_operands; i++)
{
+ rtx op_mem = extract_mem_from_operand (recog_data.operand[i]);
memcpy (op_costs[i], init_cost, struct_costs_size);
if (GET_CODE (recog_data.operand[i]) == SUBREG)
recog_data.operand[i] = SUBREG_REG (recog_data.operand[i]);
- if (MEM_P (recog_data.operand[i]))
- record_address_regs (GET_MODE (recog_data.operand[i]),
- MEM_ADDR_SPACE (recog_data.operand[i]),
- XEXP (recog_data.operand[i], 0),
+ if (MEM_P (op_mem))
+ record_address_regs (GET_MODE (op_mem),
+ MEM_ADDR_SPACE (op_mem),
+ XEXP (op_mem, 0),
0, MEM, SCRATCH, frequency * 2);
else if (constraints[i][0] == 'p'
|| (insn_extra_address_constraint
diff --git a/gcc/ira.c b/gcc/ira.c
index 27d1b3c857d..a61138c6e94 100644
--- a/gcc/ira.c
+++ b/gcc/ira.c
@@ -1868,7 +1868,7 @@ ira_setup_alts (rtx_insn *insn)
case CT_MEMORY:
case CT_SPECIAL_MEMORY:
- if (MEM_P (op))
+ if (MEM_P (extract_mem_from_operand (op)))
goto op_success;
win_p = true;
break;
diff --git a/gcc/lra-constraints.c b/gcc/lra-constraints.c
index f761d7dfe3c..b5c010d5030 100644
--- a/gcc/lra-constraints.c
+++ b/gcc/lra-constraints.c
@@ -416,14 +416,34 @@ valid_address_p (rtx op, struct address_info *ad,
return valid_address_p (ad->mode, *ad->outer, ad->as);
}
+/* 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)
+{
+ for (rtx x = op;; x = XEXP (x, 0))
+ {
+ if (MEM_P (x))
+ return x;
+ if (GET_RTX_LENGTH (GET_CODE (x)) != 1
+ || GET_RTX_FORMAT (GET_CODE (x))[0] != 'e')
+ break;
+ }
+ return op;
+}
+
/* Return true if the eliminated form of memory reference OP satisfies
extra (special) memory constraint CONSTRAINT. */
static bool
satisfies_memory_constraint_p (rtx op, enum constraint_num constraint)
{
struct address_info ad;
+ rtx mem = extract_mem_from_operand (op);
+ if (!MEM_P (mem))
+ return false;
- decompose_mem_address (&ad, op);
+ decompose_mem_address (&ad, mem);
address_eliminator eliminator (&ad);
return constraint_satisfied_p (op, constraint);
}
@@ -2406,8 +2426,7 @@ process_alt_operands (int only_alternative)
break;
case CT_MEMORY:
- if (MEM_P (op)
- && satisfies_memory_constraint_p (op, cn))
+ if (satisfies_memory_constraint_p (op, cn))
win = true;
else if (spilled_pseudo_p (op))
win = true;
@@ -2448,8 +2467,7 @@ process_alt_operands (int only_alternative)
break;
case CT_SPECIAL_MEMORY:
- if (MEM_P (op)
- && satisfies_memory_constraint_p (op, cn))
+ if (satisfies_memory_constraint_p (op, cn))
win = true;
else if (spilled_pseudo_p (op))
win = true;
diff --git a/gcc/recog.c b/gcc/recog.c
index ce83b7f5218..d3552ec63eb 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -1798,7 +1798,8 @@ asm_operand_ok (rtx op, const char *constraint, const char **constraints)
case CT_MEMORY:
case CT_SPECIAL_MEMORY:
/* Every memory operand can be reloaded to fit. */
- result = result || memory_operand (op, VOIDmode);
+ result = result || memory_operand (extract_mem_from_operand (op),
+ VOIDmode);
break;
case CT_ADDRESS:
@@ -2584,7 +2585,9 @@ constrain_operands (int strict, alternative_mask alternatives)
/* A unary operator may be accepted by the predicate, but it
is irrelevant for matching constraints. */
- if (UNARY_P (op))
+ /* For special_memory_operand, there could be a memory operand inside,
+ and it would cause a mismatch for constraint_satisfied_p. */
+ if (UNARY_P (op) && op == extract_mem_from_operand (op))
op = XEXP (op, 0);
if (GET_CODE (op) == SUBREG)
diff --git a/gcc/rtl.h b/gcc/rtl.h
index 0872cc408eb..fcec9dc6387 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -4324,6 +4324,7 @@ extern rtx gen_hard_reg_clobber (machine_mode, unsigned int);
extern rtx get_reg_known_value (unsigned int);
extern bool get_reg_known_equiv_p (unsigned int);
extern rtx get_reg_base_value (unsigned int);
+extern rtx extract_mem_from_operand (rtx);
#ifdef STACK_REGS
extern int stack_regs_mentioned (const_rtx insn);
--
2.18.1
