diff mbox series

[v3,2/5] Match: Add interface match_cond_with_binary_phi for true/false arg

Message ID 20240911062945.3358247-2-pan2.li@intel.com
State New
Headers show
Series [v3,1/5] Genmatch: Add control flow graph match for case 0 and case 1 | expand

Commit Message

Li, Pan2 Sept. 11, 2024, 6:29 a.m. UTC 
From: Pan Li <pan2.li@intel.com>

When matching the cond with 2 args phi node, we need to figure out
which arg of phi node comes from the true edge of cond block, as
well as the false edge.  This patch would like to add interface
to perform the action and return the true and false arg in TREE type.

There will be some additional handling if one of the arg is INTEGER_CST.
Because the INTEGER_CST args may have no source block, thus its' edge
source points to the condition block.  See below example in line 31,
the 255 INTEGER_CST has block 2 as source.  Thus, we need to find
the non-INTEGER_CST (aka _1) to tell which one is the true/false edge.
For example, the _1(3) takes block 3 as source, which is the dest
of false edge of the condition block.

   4   │ __attribute__((noinline))
   5   │ uint8_t sat_u_add_imm_type_check_uint8_t_fmt_2 (uint8_t x)
   6   │ {
   7   │   unsigned char _1;
   8   │   unsigned char _2;
   9   │   uint8_t _3;
  10   │   __complex__ unsigned char _5;
  11   │
  12   │ ;;   basic block 2, loop depth 0
  13   │ ;;    pred:       ENTRY
  14   │   _5 = .ADD_OVERFLOW (x_4(D), 9);
  15   │   _2 = IMAGPART_EXPR <_5>;
  16   │   if (_2 != 0)
  17   │     goto <bb 4>; [35.00%]
  18   │   else
  19   │     goto <bb 3>; [65.00%]
  20   │ ;;    succ:       3
  21   │ ;;                4
  22   │
  23   │ ;;   basic block 3, loop depth 0
  24   │ ;;    pred:       2
  25   │   _1 = REALPART_EXPR <_5>;
  26   │ ;;    succ:       4
  27   │
  28   │ ;;   basic block 4, loop depth 0
  29   │ ;;    pred:       2
  30   │ ;;                3
  31   │   # _3 = PHI <255(2), _1(3)>
  32   │   return _3;
  33   │ ;;    succ:       EXIT
  34   │
  35   │ }

The below test suites are passed for this patch.
* The rv64gcv fully regression test.
* The x86 bootstrap test.
* The x86 fully regression test.

gcc/ChangeLog:

	* gimple-match-head.cc (match_cond_with_binary_phi): Add new func
	impl to match binary phi for true and false arg.

Signed-off-by: Pan Li <pan2.li@intel.com>
---
 gcc/gimple-match-head.cc | 60 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

Comments

Richard Biener Sept. 11, 2024, 1:39 p.m. UTC | #1 
On Wed, Sep 11, 2024 at 8:31 AM <pan2.li@intel.com> wrote:
>
> From: Pan Li <pan2.li@intel.com>
>
> When matching the cond with 2 args phi node, we need to figure out
> which arg of phi node comes from the true edge of cond block, as
> well as the false edge.  This patch would like to add interface
> to perform the action and return the true and false arg in TREE type.
>
> There will be some additional handling if one of the arg is INTEGER_CST.
> Because the INTEGER_CST args may have no source block, thus its' edge
> source points to the condition block.  See below example in line 31,
> the 255 INTEGER_CST has block 2 as source.  Thus, we need to find
> the non-INTEGER_CST (aka _1) to tell which one is the true/false edge.
> For example, the _1(3) takes block 3 as source, which is the dest
> of false edge of the condition block.
>
>    4   │ __attribute__((noinline))
>    5   │ uint8_t sat_u_add_imm_type_check_uint8_t_fmt_2 (uint8_t x)
>    6   │ {
>    7   │   unsigned char _1;
>    8   │   unsigned char _2;
>    9   │   uint8_t _3;
>   10   │   __complex__ unsigned char _5;
>   11   │
>   12   │ ;;   basic block 2, loop depth 0
>   13   │ ;;    pred:       ENTRY
>   14   │   _5 = .ADD_OVERFLOW (x_4(D), 9);
>   15   │   _2 = IMAGPART_EXPR <_5>;
>   16   │   if (_2 != 0)
>   17   │     goto <bb 4>; [35.00%]
>   18   │   else
>   19   │     goto <bb 3>; [65.00%]
>   20   │ ;;    succ:       3
>   21   │ ;;                4
>   22   │
>   23   │ ;;   basic block 3, loop depth 0
>   24   │ ;;    pred:       2
>   25   │   _1 = REALPART_EXPR <_5>;
>   26   │ ;;    succ:       4
>   27   │
>   28   │ ;;   basic block 4, loop depth 0
>   29   │ ;;    pred:       2
>   30   │ ;;                3
>   31   │   # _3 = PHI <255(2), _1(3)>
>   32   │   return _3;
>   33   │ ;;    succ:       EXIT
>   34   │
>   35   │ }
>
> The below test suites are passed for this patch.
> * The rv64gcv fully regression test.
> * The x86 bootstrap test.
> * The x86 fully regression test.
>
> gcc/ChangeLog:
>
>         * gimple-match-head.cc (match_cond_with_binary_phi): Add new func
>         impl to match binary phi for true and false arg.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
>  gcc/gimple-match-head.cc | 60 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/gcc/gimple-match-head.cc b/gcc/gimple-match-head.cc
> index c51728ae742..64f4f28cc72 100644
> --- a/gcc/gimple-match-head.cc
> +++ b/gcc/gimple-match-head.cc
> @@ -490,3 +490,63 @@ match_control_flow_graph_case_1 (basic_block b3, basic_block *b_out)
>    *b_out = b0;
>    return true;
>  }
> +
> +/*
> + * Return the relevant gcond * of the given phi, as well as the true
> + * and false TREE args of the phi.  Or return NULL.
> + *
> + * If matched the gcond *, the output argument TREE true_arg and false_arg
> + * will be updated to the relevant args of phi.
> + *
> + * If failed to match, NULL gcond * will be returned, as well as the output
> + * arguments will be set to NULL_TREE.
> + */
> +
> +static inline gcond *
> +match_cond_with_binary_phi (gphi *phi, tree *true_arg, tree *false_arg)
> +{
> +  basic_block cond_block;
> +  *true_arg = *false_arg = NULL_TREE;
> +
> +  if (gimple_phi_num_args (phi) != 2)
> +    return NULL;
> +
> +  if (!match_control_flow_graph_case_0 (gimple_bb (phi), &cond_block)
> +      && !match_control_flow_graph_case_1 (gimple_bb (phi), &cond_block))
> +    return NULL;
> +
> +  gcond *cond = safe_dyn_cast <gcond *> (*gsi_last_bb (cond_block));
> +
> +  if (!cond || EDGE_COUNT (cond_block->succs) != 2)
> +    return NULL;
> +
> +  tree t0 = gimple_phi_arg_def (phi, 0);
> +  tree t1 = gimple_phi_arg_def (phi, 1);
> +  edge e0 = gimple_phi_arg_edge (phi, 0);
> +  edge e1 = gimple_phi_arg_edge (phi, 1);
> +
> +  if (TREE_CODE (t0) == INTEGER_CST && TREE_CODE (t1) == INTEGER_CST)
> +    return NULL;
> +
> +  bool arg_0_cst_p = TREE_CODE (t0) == INTEGER_CST;
> +  edge arg_edge = arg_0_cst_p ? e1 : e0;
> +  tree arg = arg_0_cst_p ? t1 : t0;
> +  tree other_arg = arg_0_cst_p ? t0 : t1;

why would arg_edge depend on whether t0 is INTEGER_CST or not?

> +  edge cond_e0 = EDGE_SUCC (cond_block, 0);
> +  edge cond_e1 = EDGE_SUCC (cond_block, 1);
> +  edge matched_edge = arg_edge->src == cond_e0->dest ? cond_e0 : cond_e1;
> +
> +  if (matched_edge->flags & EDGE_TRUE_VALUE)

I don't think this works reliably like for case 0 if cond_e0 leads to
the PHI block?

Can you instead inline match_control_flow_graph_case_0 and _1 and do the
argument assignment within the three cases of CFGs we accept?  That
would be much easier to follow.

> +    {
> +      *true_arg = arg;
> +      *false_arg = other_arg;
> +    }
> +  else
> +    {
> +      *false_arg = arg;
> +      *true_arg = other_arg;
> +    }
> +
> +  return cond;
> +}
> --
> 2.43.0
>
Li, Pan2 Sept. 12, 2024, 1:40 a.m. UTC | #2 
Thanks Richard for comments.

> why would arg_edge depend on whether t0 is INTEGER_CST or not?
Because the edge->src of INTEGER_CST points to the cond block which cannot match the 
edge->dest of the cond_block. For example as below, the first arg of PHI is 255(2), which 
cannot match neither goto <bb 4> nor goto <bb 3>.

Thus, I need to take the second arg, aka _1(3) to match the edge->dest of cond_block.
Aka the phi arg edge->src == cond_block edge->dest. In below example,
the goto<bb 3> matches _1(3) with false condition, and then I can locate the edge from b2 -> b3.

Or is there any better approach for this scenario?

   4   │ __attribute__((noinline))
   5   │ uint8_t sat_u_add_imm_type_check_uint8_t_fmt_2 (uint8_t x)
   6   │ {
   7   │   unsigned char _1;
   8   │   unsigned char _2;
   9   │   uint8_t _3;
  10   │   __complex__ unsigned char _5;
  11   │
  12   │ ;;   basic block 2, loop depth 0
  13   │ ;;    pred:       ENTRY
  14   │   _5 = .ADD_OVERFLOW (x_4(D), 9);
  15   │   _2 = IMAGPART_EXPR <_5>;
  16   │   if (_2 != 0)
  17   │     goto <bb 4>; [35.00%]
  18   │   else
  19   │     goto <bb 3>; [65.00%]
  20   │ ;;    succ:       3
  21   │ ;;                4
  22   │
  23   │ ;;   basic block 3, loop depth 0
  24   │ ;;    pred:       2
  25   │   _1 = REALPART_EXPR <_5>;
  26   │ ;;    succ:       4
  27   │
  28   │ ;;   basic block 4, loop depth 0
  29   │ ;;    pred:       2
  30   │ ;;                3
  31   │   # _3 = PHI <255(2), _1(3)>
  32   │   return _3;
  33   │ ;;    succ:       EXIT
  34   │
  35   │ }

> Can you instead inline match_control_flow_graph_case_0 and _1 and do the
> argument assignment within the three cases of CFGs we accept?  That
> would be much easier to follow.

To double confirm, are you suggest inline the cfg match for both the case_0 and case_1?
That may make func body grows, and we may have more cases like case_2, case_3... etc.
If so, I will inline this to match_cond_with_binary_phi in v4.

Pan

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Wednesday, September 11, 2024 9:39 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@gmail.com
Subject: Re: [PATCH v3 2/5] Match: Add interface match_cond_with_binary_phi for true/false arg

On Wed, Sep 11, 2024 at 8:31 AM <pan2.li@intel.com> wrote:
>
> From: Pan Li <pan2.li@intel.com>
>
> When matching the cond with 2 args phi node, we need to figure out
> which arg of phi node comes from the true edge of cond block, as
> well as the false edge.  This patch would like to add interface
> to perform the action and return the true and false arg in TREE type.
>
> There will be some additional handling if one of the arg is INTEGER_CST.
> Because the INTEGER_CST args may have no source block, thus its' edge
> source points to the condition block.  See below example in line 31,
> the 255 INTEGER_CST has block 2 as source.  Thus, we need to find
> the non-INTEGER_CST (aka _1) to tell which one is the true/false edge.
> For example, the _1(3) takes block 3 as source, which is the dest
> of false edge of the condition block.
>
>    4   │ __attribute__((noinline))
>    5   │ uint8_t sat_u_add_imm_type_check_uint8_t_fmt_2 (uint8_t x)
>    6   │ {
>    7   │   unsigned char _1;
>    8   │   unsigned char _2;
>    9   │   uint8_t _3;
>   10   │   __complex__ unsigned char _5;
>   11   │
>   12   │ ;;   basic block 2, loop depth 0
>   13   │ ;;    pred:       ENTRY
>   14   │   _5 = .ADD_OVERFLOW (x_4(D), 9);
>   15   │   _2 = IMAGPART_EXPR <_5>;
>   16   │   if (_2 != 0)
>   17   │     goto <bb 4>; [35.00%]
>   18   │   else
>   19   │     goto <bb 3>; [65.00%]
>   20   │ ;;    succ:       3
>   21   │ ;;                4
>   22   │
>   23   │ ;;   basic block 3, loop depth 0
>   24   │ ;;    pred:       2
>   25   │   _1 = REALPART_EXPR <_5>;
>   26   │ ;;    succ:       4
>   27   │
>   28   │ ;;   basic block 4, loop depth 0
>   29   │ ;;    pred:       2
>   30   │ ;;                3
>   31   │   # _3 = PHI <255(2), _1(3)>
>   32   │   return _3;
>   33   │ ;;    succ:       EXIT
>   34   │
>   35   │ }
>
> The below test suites are passed for this patch.
> * The rv64gcv fully regression test.
> * The x86 bootstrap test.
> * The x86 fully regression test.
>
> gcc/ChangeLog:
>
>         * gimple-match-head.cc (match_cond_with_binary_phi): Add new func
>         impl to match binary phi for true and false arg.
>
> Signed-off-by: Pan Li <pan2.li@intel.com>
> ---
>  gcc/gimple-match-head.cc | 60 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
>
> diff --git a/gcc/gimple-match-head.cc b/gcc/gimple-match-head.cc
> index c51728ae742..64f4f28cc72 100644
> --- a/gcc/gimple-match-head.cc
> +++ b/gcc/gimple-match-head.cc
> @@ -490,3 +490,63 @@ match_control_flow_graph_case_1 (basic_block b3, basic_block *b_out)
>    *b_out = b0;
>    return true;
>  }
> +
> +/*
> + * Return the relevant gcond * of the given phi, as well as the true
> + * and false TREE args of the phi.  Or return NULL.
> + *
> + * If matched the gcond *, the output argument TREE true_arg and false_arg
> + * will be updated to the relevant args of phi.
> + *
> + * If failed to match, NULL gcond * will be returned, as well as the output
> + * arguments will be set to NULL_TREE.
> + */
> +
> +static inline gcond *
> +match_cond_with_binary_phi (gphi *phi, tree *true_arg, tree *false_arg)
> +{
> +  basic_block cond_block;
> +  *true_arg = *false_arg = NULL_TREE;
> +
> +  if (gimple_phi_num_args (phi) != 2)
> +    return NULL;
> +
> +  if (!match_control_flow_graph_case_0 (gimple_bb (phi), &cond_block)
> +      && !match_control_flow_graph_case_1 (gimple_bb (phi), &cond_block))
> +    return NULL;
> +
> +  gcond *cond = safe_dyn_cast <gcond *> (*gsi_last_bb (cond_block));
> +
> +  if (!cond || EDGE_COUNT (cond_block->succs) != 2)
> +    return NULL;
> +
> +  tree t0 = gimple_phi_arg_def (phi, 0);
> +  tree t1 = gimple_phi_arg_def (phi, 1);
> +  edge e0 = gimple_phi_arg_edge (phi, 0);
> +  edge e1 = gimple_phi_arg_edge (phi, 1);
> +
> +  if (TREE_CODE (t0) == INTEGER_CST && TREE_CODE (t1) == INTEGER_CST)
> +    return NULL;
> +
> +  bool arg_0_cst_p = TREE_CODE (t0) == INTEGER_CST;
> +  edge arg_edge = arg_0_cst_p ? e1 : e0;
> +  tree arg = arg_0_cst_p ? t1 : t0;
> +  tree other_arg = arg_0_cst_p ? t0 : t1;

why would arg_edge depend on whether t0 is INTEGER_CST or not?

> +  edge cond_e0 = EDGE_SUCC (cond_block, 0);
> +  edge cond_e1 = EDGE_SUCC (cond_block, 1);
> +  edge matched_edge = arg_edge->src == cond_e0->dest ? cond_e0 : cond_e1;
> +
> +  if (matched_edge->flags & EDGE_TRUE_VALUE)

I don't think this works reliably like for case 0 if cond_e0 leads to
the PHI block?

Can you instead inline match_control_flow_graph_case_0 and _1 and do the
argument assignment within the three cases of CFGs we accept?  That
would be much easier to follow.

> +    {
> +      *true_arg = arg;
> +      *false_arg = other_arg;
> +    }
> +  else
> +    {
> +      *false_arg = arg;
> +      *true_arg = other_arg;
> +    }
> +
> +  return cond;
> +}
> --
> 2.43.0
>
Richard Biener Sept. 12, 2024, 6:50 a.m. UTC | #3 
On Thu, Sep 12, 2024 at 3:41 AM Li, Pan2 <pan2.li@intel.com> wrote:
>
> Thanks Richard for comments.
>
> > why would arg_edge depend on whether t0 is INTEGER_CST or not?
> Because the edge->src of INTEGER_CST points to the cond block which cannot match the
> edge->dest of the cond_block. For example as below, the first arg of PHI is 255(2), which
> cannot match neither goto <bb 4> nor goto <bb 3>.
>
> Thus, I need to take the second arg, aka _1(3) to match the edge->dest of cond_block.
> Aka the phi arg edge->src == cond_block edge->dest. In below example,
> the goto<bb 3> matches _1(3) with false condition, and then I can locate the edge from b2 -> b3.
>
> Or is there any better approach for this scenario?
>
>    4   │ __attribute__((noinline))
>    5   │ uint8_t sat_u_add_imm_type_check_uint8_t_fmt_2 (uint8_t x)
>    6   │ {
>    7   │   unsigned char _1;
>    8   │   unsigned char _2;
>    9   │   uint8_t _3;
>   10   │   __complex__ unsigned char _5;
>   11   │
>   12   │ ;;   basic block 2, loop depth 0
>   13   │ ;;    pred:       ENTRY
>   14   │   _5 = .ADD_OVERFLOW (x_4(D), 9);
>   15   │   _2 = IMAGPART_EXPR <_5>;
>   16   │   if (_2 != 0)
>   17   │     goto <bb 4>; [35.00%]
>   18   │   else
>   19   │     goto <bb 3>; [65.00%]
>   20   │ ;;    succ:       3
>   21   │ ;;                4
>   22   │
>   23   │ ;;   basic block 3, loop depth 0
>   24   │ ;;    pred:       2
>   25   │   _1 = REALPART_EXPR <_5>;
>   26   │ ;;    succ:       4
>   27   │
>   28   │ ;;   basic block 4, loop depth 0
>   29   │ ;;    pred:       2
>   30   │ ;;                3
>   31   │   # _3 = PHI <255(2), _1(3)>
>   32   │   return _3;
>   33   │ ;;    succ:       EXIT
>   34   │
>   35   │ }
>
> > Can you instead inline match_control_flow_graph_case_0 and _1 and do the
> > argument assignment within the three cases of CFGs we accept?  That
> > would be much easier to follow.
>
> To double confirm, are you suggest inline the cfg match for both the case_0 and case_1?
> That may make func body grows, and we may have more cases like case_2, case_3... etc.
> If so, I will inline this to match_cond_with_binary_phi in v4.

Yes, inline both CFG matches and unify them - there should be exactly
three cases at
the moment.  And "duplicate" computing the true/false arg into the
respective cases
since it's trivial which edge(s) to look at.

This should make the code more maintainable and easier to understand.

I'm not sure what additional cases you are thinking of, more complex CFGs should
always mean more than a single controlling condition - I'm not sure we
want to go
the way to present those as cond1 | cond2.

Richard.

> Pan
>
> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Wednesday, September 11, 2024 9:39 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@gmail.com
> Subject: Re: [PATCH v3 2/5] Match: Add interface match_cond_with_binary_phi for true/false arg
>
> On Wed, Sep 11, 2024 at 8:31 AM <pan2.li@intel.com> wrote:
> >
> > From: Pan Li <pan2.li@intel.com>
> >
> > When matching the cond with 2 args phi node, we need to figure out
> > which arg of phi node comes from the true edge of cond block, as
> > well as the false edge.  This patch would like to add interface
> > to perform the action and return the true and false arg in TREE type.
> >
> > There will be some additional handling if one of the arg is INTEGER_CST.
> > Because the INTEGER_CST args may have no source block, thus its' edge
> > source points to the condition block.  See below example in line 31,
> > the 255 INTEGER_CST has block 2 as source.  Thus, we need to find
> > the non-INTEGER_CST (aka _1) to tell which one is the true/false edge.
> > For example, the _1(3) takes block 3 as source, which is the dest
> > of false edge of the condition block.
> >
> >    4   │ __attribute__((noinline))
> >    5   │ uint8_t sat_u_add_imm_type_check_uint8_t_fmt_2 (uint8_t x)
> >    6   │ {
> >    7   │   unsigned char _1;
> >    8   │   unsigned char _2;
> >    9   │   uint8_t _3;
> >   10   │   __complex__ unsigned char _5;
> >   11   │
> >   12   │ ;;   basic block 2, loop depth 0
> >   13   │ ;;    pred:       ENTRY
> >   14   │   _5 = .ADD_OVERFLOW (x_4(D), 9);
> >   15   │   _2 = IMAGPART_EXPR <_5>;
> >   16   │   if (_2 != 0)
> >   17   │     goto <bb 4>; [35.00%]
> >   18   │   else
> >   19   │     goto <bb 3>; [65.00%]
> >   20   │ ;;    succ:       3
> >   21   │ ;;                4
> >   22   │
> >   23   │ ;;   basic block 3, loop depth 0
> >   24   │ ;;    pred:       2
> >   25   │   _1 = REALPART_EXPR <_5>;
> >   26   │ ;;    succ:       4
> >   27   │
> >   28   │ ;;   basic block 4, loop depth 0
> >   29   │ ;;    pred:       2
> >   30   │ ;;                3
> >   31   │   # _3 = PHI <255(2), _1(3)>
> >   32   │   return _3;
> >   33   │ ;;    succ:       EXIT
> >   34   │
> >   35   │ }
> >
> > The below test suites are passed for this patch.
> > * The rv64gcv fully regression test.
> > * The x86 bootstrap test.
> > * The x86 fully regression test.
> >
> > gcc/ChangeLog:
> >
> >         * gimple-match-head.cc (match_cond_with_binary_phi): Add new func
> >         impl to match binary phi for true and false arg.
> >
> > Signed-off-by: Pan Li <pan2.li@intel.com>
> > ---
> >  gcc/gimple-match-head.cc | 60 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >
> > diff --git a/gcc/gimple-match-head.cc b/gcc/gimple-match-head.cc
> > index c51728ae742..64f4f28cc72 100644
> > --- a/gcc/gimple-match-head.cc
> > +++ b/gcc/gimple-match-head.cc
> > @@ -490,3 +490,63 @@ match_control_flow_graph_case_1 (basic_block b3, basic_block *b_out)
> >    *b_out = b0;
> >    return true;
> >  }
> > +
> > +/*
> > + * Return the relevant gcond * of the given phi, as well as the true
> > + * and false TREE args of the phi.  Or return NULL.
> > + *
> > + * If matched the gcond *, the output argument TREE true_arg and false_arg
> > + * will be updated to the relevant args of phi.
> > + *
> > + * If failed to match, NULL gcond * will be returned, as well as the output
> > + * arguments will be set to NULL_TREE.
> > + */
> > +
> > +static inline gcond *
> > +match_cond_with_binary_phi (gphi *phi, tree *true_arg, tree *false_arg)
> > +{
> > +  basic_block cond_block;
> > +  *true_arg = *false_arg = NULL_TREE;
> > +
> > +  if (gimple_phi_num_args (phi) != 2)
> > +    return NULL;
> > +
> > +  if (!match_control_flow_graph_case_0 (gimple_bb (phi), &cond_block)
> > +      && !match_control_flow_graph_case_1 (gimple_bb (phi), &cond_block))
> > +    return NULL;
> > +
> > +  gcond *cond = safe_dyn_cast <gcond *> (*gsi_last_bb (cond_block));
> > +
> > +  if (!cond || EDGE_COUNT (cond_block->succs) != 2)
> > +    return NULL;
> > +
> > +  tree t0 = gimple_phi_arg_def (phi, 0);
> > +  tree t1 = gimple_phi_arg_def (phi, 1);
> > +  edge e0 = gimple_phi_arg_edge (phi, 0);
> > +  edge e1 = gimple_phi_arg_edge (phi, 1);
> > +
> > +  if (TREE_CODE (t0) == INTEGER_CST && TREE_CODE (t1) == INTEGER_CST)
> > +    return NULL;
> > +
> > +  bool arg_0_cst_p = TREE_CODE (t0) == INTEGER_CST;
> > +  edge arg_edge = arg_0_cst_p ? e1 : e0;
> > +  tree arg = arg_0_cst_p ? t1 : t0;
> > +  tree other_arg = arg_0_cst_p ? t0 : t1;
>
> why would arg_edge depend on whether t0 is INTEGER_CST or not?
>
> > +  edge cond_e0 = EDGE_SUCC (cond_block, 0);
> > +  edge cond_e1 = EDGE_SUCC (cond_block, 1);
> > +  edge matched_edge = arg_edge->src == cond_e0->dest ? cond_e0 : cond_e1;
> > +
> > +  if (matched_edge->flags & EDGE_TRUE_VALUE)
>
> I don't think this works reliably like for case 0 if cond_e0 leads to
> the PHI block?
>
> Can you instead inline match_control_flow_graph_case_0 and _1 and do the
> argument assignment within the three cases of CFGs we accept?  That
> would be much easier to follow.
>
> > +    {
> > +      *true_arg = arg;
> > +      *false_arg = other_arg;
> > +    }
> > +  else
> > +    {
> > +      *false_arg = arg;
> > +      *true_arg = other_arg;
> > +    }
> > +
> > +  return cond;
> > +}
> > --
> > 2.43.0
> >
Li, Pan2 Sept. 12, 2024, 6:54 a.m. UTC | #4 
Thanks Richard for comments.

> Yes, inline both CFG matches and unify them - there should be exactly
> three cases at
> the moment.  And "duplicate" computing the true/false arg into the
> respective cases
> since it's trivial which edge(s) to look at.

Got it, will resend the v4 series for this change.

Pan

-----Original Message-----
From: Richard Biener <richard.guenther@gmail.com> 
Sent: Thursday, September 12, 2024 2:51 PM
To: Li, Pan2 <pan2.li@intel.com>
Cc: gcc-patches@gcc.gnu.org; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@gmail.com
Subject: Re: [PATCH v3 2/5] Match: Add interface match_cond_with_binary_phi for true/false arg

On Thu, Sep 12, 2024 at 3:41 AM Li, Pan2 <pan2.li@intel.com> wrote:
>
> Thanks Richard for comments.
>
> > why would arg_edge depend on whether t0 is INTEGER_CST or not?
> Because the edge->src of INTEGER_CST points to the cond block which cannot match the
> edge->dest of the cond_block. For example as below, the first arg of PHI is 255(2), which
> cannot match neither goto <bb 4> nor goto <bb 3>.
>
> Thus, I need to take the second arg, aka _1(3) to match the edge->dest of cond_block.
> Aka the phi arg edge->src == cond_block edge->dest. In below example,
> the goto<bb 3> matches _1(3) with false condition, and then I can locate the edge from b2 -> b3.
>
> Or is there any better approach for this scenario?
>
>    4   │ __attribute__((noinline))
>    5   │ uint8_t sat_u_add_imm_type_check_uint8_t_fmt_2 (uint8_t x)
>    6   │ {
>    7   │   unsigned char _1;
>    8   │   unsigned char _2;
>    9   │   uint8_t _3;
>   10   │   __complex__ unsigned char _5;
>   11   │
>   12   │ ;;   basic block 2, loop depth 0
>   13   │ ;;    pred:       ENTRY
>   14   │   _5 = .ADD_OVERFLOW (x_4(D), 9);
>   15   │   _2 = IMAGPART_EXPR <_5>;
>   16   │   if (_2 != 0)
>   17   │     goto <bb 4>; [35.00%]
>   18   │   else
>   19   │     goto <bb 3>; [65.00%]
>   20   │ ;;    succ:       3
>   21   │ ;;                4
>   22   │
>   23   │ ;;   basic block 3, loop depth 0
>   24   │ ;;    pred:       2
>   25   │   _1 = REALPART_EXPR <_5>;
>   26   │ ;;    succ:       4
>   27   │
>   28   │ ;;   basic block 4, loop depth 0
>   29   │ ;;    pred:       2
>   30   │ ;;                3
>   31   │   # _3 = PHI <255(2), _1(3)>
>   32   │   return _3;
>   33   │ ;;    succ:       EXIT
>   34   │
>   35   │ }
>
> > Can you instead inline match_control_flow_graph_case_0 and _1 and do the
> > argument assignment within the three cases of CFGs we accept?  That
> > would be much easier to follow.
>
> To double confirm, are you suggest inline the cfg match for both the case_0 and case_1?
> That may make func body grows, and we may have more cases like case_2, case_3... etc.
> If so, I will inline this to match_cond_with_binary_phi in v4.

Yes, inline both CFG matches and unify them - there should be exactly
three cases at
the moment.  And "duplicate" computing the true/false arg into the
respective cases
since it's trivial which edge(s) to look at.

This should make the code more maintainable and easier to understand.

I'm not sure what additional cases you are thinking of, more complex CFGs should
always mean more than a single controlling condition - I'm not sure we
want to go
the way to present those as cond1 | cond2.

Richard.

> Pan
>
> -----Original Message-----
> From: Richard Biener <richard.guenther@gmail.com>
> Sent: Wednesday, September 11, 2024 9:39 PM
> To: Li, Pan2 <pan2.li@intel.com>
> Cc: gcc-patches@gcc.gnu.org; Tamar.Christina@arm.com; juzhe.zhong@rivai.ai; kito.cheng@gmail.com; jeffreyalaw@gmail.com; rdapp.gcc@gmail.com
> Subject: Re: [PATCH v3 2/5] Match: Add interface match_cond_with_binary_phi for true/false arg
>
> On Wed, Sep 11, 2024 at 8:31 AM <pan2.li@intel.com> wrote:
> >
> > From: Pan Li <pan2.li@intel.com>
> >
> > When matching the cond with 2 args phi node, we need to figure out
> > which arg of phi node comes from the true edge of cond block, as
> > well as the false edge.  This patch would like to add interface
> > to perform the action and return the true and false arg in TREE type.
> >
> > There will be some additional handling if one of the arg is INTEGER_CST.
> > Because the INTEGER_CST args may have no source block, thus its' edge
> > source points to the condition block.  See below example in line 31,
> > the 255 INTEGER_CST has block 2 as source.  Thus, we need to find
> > the non-INTEGER_CST (aka _1) to tell which one is the true/false edge.
> > For example, the _1(3) takes block 3 as source, which is the dest
> > of false edge of the condition block.
> >
> >    4   │ __attribute__((noinline))
> >    5   │ uint8_t sat_u_add_imm_type_check_uint8_t_fmt_2 (uint8_t x)
> >    6   │ {
> >    7   │   unsigned char _1;
> >    8   │   unsigned char _2;
> >    9   │   uint8_t _3;
> >   10   │   __complex__ unsigned char _5;
> >   11   │
> >   12   │ ;;   basic block 2, loop depth 0
> >   13   │ ;;    pred:       ENTRY
> >   14   │   _5 = .ADD_OVERFLOW (x_4(D), 9);
> >   15   │   _2 = IMAGPART_EXPR <_5>;
> >   16   │   if (_2 != 0)
> >   17   │     goto <bb 4>; [35.00%]
> >   18   │   else
> >   19   │     goto <bb 3>; [65.00%]
> >   20   │ ;;    succ:       3
> >   21   │ ;;                4
> >   22   │
> >   23   │ ;;   basic block 3, loop depth 0
> >   24   │ ;;    pred:       2
> >   25   │   _1 = REALPART_EXPR <_5>;
> >   26   │ ;;    succ:       4
> >   27   │
> >   28   │ ;;   basic block 4, loop depth 0
> >   29   │ ;;    pred:       2
> >   30   │ ;;                3
> >   31   │   # _3 = PHI <255(2), _1(3)>
> >   32   │   return _3;
> >   33   │ ;;    succ:       EXIT
> >   34   │
> >   35   │ }
> >
> > The below test suites are passed for this patch.
> > * The rv64gcv fully regression test.
> > * The x86 bootstrap test.
> > * The x86 fully regression test.
> >
> > gcc/ChangeLog:
> >
> >         * gimple-match-head.cc (match_cond_with_binary_phi): Add new func
> >         impl to match binary phi for true and false arg.
> >
> > Signed-off-by: Pan Li <pan2.li@intel.com>
> > ---
> >  gcc/gimple-match-head.cc | 60 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >
> > diff --git a/gcc/gimple-match-head.cc b/gcc/gimple-match-head.cc
> > index c51728ae742..64f4f28cc72 100644
> > --- a/gcc/gimple-match-head.cc
> > +++ b/gcc/gimple-match-head.cc
> > @@ -490,3 +490,63 @@ match_control_flow_graph_case_1 (basic_block b3, basic_block *b_out)
> >    *b_out = b0;
> >    return true;
> >  }
> > +
> > +/*
> > + * Return the relevant gcond * of the given phi, as well as the true
> > + * and false TREE args of the phi.  Or return NULL.
> > + *
> > + * If matched the gcond *, the output argument TREE true_arg and false_arg
> > + * will be updated to the relevant args of phi.
> > + *
> > + * If failed to match, NULL gcond * will be returned, as well as the output
> > + * arguments will be set to NULL_TREE.
> > + */
> > +
> > +static inline gcond *
> > +match_cond_with_binary_phi (gphi *phi, tree *true_arg, tree *false_arg)
> > +{
> > +  basic_block cond_block;
> > +  *true_arg = *false_arg = NULL_TREE;
> > +
> > +  if (gimple_phi_num_args (phi) != 2)
> > +    return NULL;
> > +
> > +  if (!match_control_flow_graph_case_0 (gimple_bb (phi), &cond_block)
> > +      && !match_control_flow_graph_case_1 (gimple_bb (phi), &cond_block))
> > +    return NULL;
> > +
> > +  gcond *cond = safe_dyn_cast <gcond *> (*gsi_last_bb (cond_block));
> > +
> > +  if (!cond || EDGE_COUNT (cond_block->succs) != 2)
> > +    return NULL;
> > +
> > +  tree t0 = gimple_phi_arg_def (phi, 0);
> > +  tree t1 = gimple_phi_arg_def (phi, 1);
> > +  edge e0 = gimple_phi_arg_edge (phi, 0);
> > +  edge e1 = gimple_phi_arg_edge (phi, 1);
> > +
> > +  if (TREE_CODE (t0) == INTEGER_CST && TREE_CODE (t1) == INTEGER_CST)
> > +    return NULL;
> > +
> > +  bool arg_0_cst_p = TREE_CODE (t0) == INTEGER_CST;
> > +  edge arg_edge = arg_0_cst_p ? e1 : e0;
> > +  tree arg = arg_0_cst_p ? t1 : t0;
> > +  tree other_arg = arg_0_cst_p ? t0 : t1;
>
> why would arg_edge depend on whether t0 is INTEGER_CST or not?
>
> > +  edge cond_e0 = EDGE_SUCC (cond_block, 0);
> > +  edge cond_e1 = EDGE_SUCC (cond_block, 1);
> > +  edge matched_edge = arg_edge->src == cond_e0->dest ? cond_e0 : cond_e1;
> > +
> > +  if (matched_edge->flags & EDGE_TRUE_VALUE)
>
> I don't think this works reliably like for case 0 if cond_e0 leads to
> the PHI block?
>
> Can you instead inline match_control_flow_graph_case_0 and _1 and do the
> argument assignment within the three cases of CFGs we accept?  That
> would be much easier to follow.
>
> > +    {
> > +      *true_arg = arg;
> > +      *false_arg = other_arg;
> > +    }
> > +  else
> > +    {
> > +      *false_arg = arg;
> > +      *true_arg = other_arg;
> > +    }
> > +
> > +  return cond;
> > +}
> > --
> > 2.43.0
> >
diff mbox series

Patch

diff --git a/gcc/gimple-match-head.cc b/gcc/gimple-match-head.cc
index c51728ae742..64f4f28cc72 100644
--- a/gcc/gimple-match-head.cc
+++ b/gcc/gimple-match-head.cc
@@ -490,3 +490,63 @@  match_control_flow_graph_case_1 (basic_block b3, basic_block *b_out)
   *b_out = b0;
   return true;
 }
+
+/*
+ * Return the relevant gcond * of the given phi, as well as the true
+ * and false TREE args of the phi.  Or return NULL.
+ *
+ * If matched the gcond *, the output argument TREE true_arg and false_arg
+ * will be updated to the relevant args of phi.
+ *
+ * If failed to match, NULL gcond * will be returned, as well as the output
+ * arguments will be set to NULL_TREE.
+ */
+
+static inline gcond *
+match_cond_with_binary_phi (gphi *phi, tree *true_arg, tree *false_arg)
+{
+  basic_block cond_block;
+  *true_arg = *false_arg = NULL_TREE;
+
+  if (gimple_phi_num_args (phi) != 2)
+    return NULL;
+
+  if (!match_control_flow_graph_case_0 (gimple_bb (phi), &cond_block)
+      && !match_control_flow_graph_case_1 (gimple_bb (phi), &cond_block))
+    return NULL;
+
+  gcond *cond = safe_dyn_cast <gcond *> (*gsi_last_bb (cond_block));
+
+  if (!cond || EDGE_COUNT (cond_block->succs) != 2)
+    return NULL;
+
+  tree t0 = gimple_phi_arg_def (phi, 0);
+  tree t1 = gimple_phi_arg_def (phi, 1);
+  edge e0 = gimple_phi_arg_edge (phi, 0);
+  edge e1 = gimple_phi_arg_edge (phi, 1);
+
+  if (TREE_CODE (t0) == INTEGER_CST && TREE_CODE (t1) == INTEGER_CST)
+    return NULL;
+
+  bool arg_0_cst_p = TREE_CODE (t0) == INTEGER_CST;
+  edge arg_edge = arg_0_cst_p ? e1 : e0;
+  tree arg = arg_0_cst_p ? t1 : t0;
+  tree other_arg = arg_0_cst_p ? t0 : t1;
+
+  edge cond_e0 = EDGE_SUCC (cond_block, 0);
+  edge cond_e1 = EDGE_SUCC (cond_block, 1);
+  edge matched_edge = arg_edge->src == cond_e0->dest ? cond_e0 : cond_e1;
+
+  if (matched_edge->flags & EDGE_TRUE_VALUE)
+    {
+      *true_arg = arg;
+      *false_arg = other_arg;
+    }
+  else
+    {
+      *false_arg = arg;
+      *true_arg = other_arg;
+    }
+
+  return cond;
+}