Re: [PATCH v3] phiopt: Add some TARGET_MEM_REF support to factoring of loads [PR100173]

On Wed, Jul 29, 2026 at 5:01 AM Richard Biener
<[email protected]> wrote:
>
> On Sat, Jul 25, 2026 at 2:48 AM Andrea Pinski
> <[email protected]> wrote:
> >
> > The testcase in factor_op_phi-load-target_mem-1.c at -O2 gets:
> > ```
> >   if (_4 > _6)
> >     goto <bb 4>; [50.00%]
> >   else
> >     goto <bb 5>; [50.00%]
> >
> >   <bb 4> [local count: 531502204]:
> >   t_21 = MEM[(int *)c_19(D) + ivtmp.23_12 * 1];
> >   goto <bb 6>; [100.00%]
> >
> >   <bb 5> [local count: 531502204]:
> >   t_20 = MEM[(int *)c_19(D) + 8B + ivtmp.23_12 * 1];
> >
> >   <bb 6> [local count: 1063004408]:
> >   # t_13 = PHI <t_21(4), t_20(5)>
> >   # t1_14 = PHI <_4(4), _6(5)>
> > ```
> >
> > But that MEM is a TARGET_MEM_REF which is not supported by
> > factor_out_conditional_load yet. This adds simple TARGET_MEM_REF
> > support by requiring the index/step and index2 to be all the same.
> > It even supports a mismatched TARGET_MEM_REF with a MEM_REF but
> > only if the TARGET_MEM_REF had an null index/step and index2.
> >
> > We now get a similar code generation for telecom/viterb00data_1 (EEMBC)
> > at -O2 as LLVM.
> >
> > Changes since v1:
> > * v2: Fix small issue checking of equality and nullptr of TARGET_MEM_REF
> > operands.
> > * v3: Fix operand_equal check.
> >
> > Bootstrapped and tested on x86_64-linux-gnu.
> >
> >         PR tree-optmization/100173
> >
> > gcc/ChangeLog:
> >
> >         * tree-ssa-phiopt.cc (factor_out_conditional_load): Add simple
> >         support for TARGET_MEM_REF.
> >
> > gcc/testsuite/ChangeLog:
> >
> >         * gcc.dg/tree-ssa/factor_op_phi-load-target_mem-1.c: New test.
> >
> > Signed-off-by: Andrea Pinski <[email protected]>
> > ---
> >  .../factor_op_phi-load-target_mem-1.c         | 24 +++++++++
> >  gcc/tree-ssa-phiopt.cc                        | 53 +++++++++++++++++--
> >  2 files changed, 74 insertions(+), 3 deletions(-)
> >  create mode 100644 gcc/testsuite/gcc.dg/tree-ssa/factor_op_phi-load-target_mem-1.c
> >
> > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/factor_op_phi-load-target_mem-1.c b/gcc/testsuite/gcc.dg/tree-ssa/factor_op_phi-load-target_mem-1.c
> > new file mode 100644
> > index 00000000000..2e2a9ad7834
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/tree-ssa/factor_op_phi-load-target_mem-1.c
> > @@ -0,0 +1,24 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -fdump-tree-phiopt4-details" } */
> > +/* PR tree-optmization/100173 */
> > +/* TARGET_MEM_REF was not being supported for load factoring.  */
> > +
> > +void f(int a, int *b, int *d, int *c)
> > +{
> > +    for(int i = 0; i < 1024; i++)
> > +    {
> > +      int t;
> > +      int t1;
> > +      if (b[i] > d[i]) {
> > +        t1 = b[i];
> > +        t = c[i];
> > +      }
> > +      else {
> > +        t1 = d[i];
> > +        t = c[i+2];
> > +      }
> > +      b[i] = t+t1;
> > +    }
> > +}
> > +
> > +/* { dg-final { scan-tree-dump "changed to factor out load from COND_EXPR" "phiopt4" } } */
> > diff --git a/gcc/tree-ssa-phiopt.cc b/gcc/tree-ssa-phiopt.cc
> > index d48f164c7fb..a5f469e883e 100644
> > --- a/gcc/tree-ssa-phiopt.cc
> > +++ b/gcc/tree-ssa-phiopt.cc
> > @@ -3972,12 +3972,54 @@ factor_out_conditional_load (edge e0, edge e1, basic_block merge, gphi *phi,
> >
> >    tree ref0 = gimple_assign_rhs1 (load0);
> >    tree ref1 = gimple_assign_rhs1 (load1);
> > +  tree index = nullptr;
> > +  tree step = nullptr;
> > +  tree index2 = nullptr;
> >
> >    /* Both must be *P loads of a compatible value type.  The
> >       TBAA alias-ptr type carried by MEM_REF operand 1 need not match; it is
> >       merged the way get_alias_type_for_stmts does when the load is built.  */
> > -  if (TREE_CODE (ref0) != MEM_REF || TREE_CODE (ref1) != MEM_REF
> > -      || !types_compatible_p (TREE_TYPE (ref0), TREE_TYPE (ref1)))
> > +  if (TREE_CODE (ref0) != MEM_REF)
> > +    {
> > +      if (TREE_CODE (ref0) != TARGET_MEM_REF)
> > +       return false;
> > +      index = TREE_OPERAND (ref0, 2);
>
> TMR_INDEX
>
> > +      step = TREE_OPERAND (ref0, 3);
>
> TMR_STEP
>
> > +      index2 = TREE_OPERAND (ref0, 4);
>
> TMR_INDEX2
>
> > +    }
> > +  if (TREE_CODE (ref1) == MEM_REF)
> > +    {
> > +      if (index || step || index2)
> > +       return false;
> > +    }
> > +  else
> > +    {
> > +      if (TREE_CODE (ref1) != TARGET_MEM_REF)
> > +       return false;
> > +      if (index != TREE_OPERAND (ref1, 2))
> > +       {
> > +         if (!index || !TREE_OPERAND (ref1, 2))
> > +           return false;
> > +         if (!operand_equal_p (index, TREE_OPERAND (ref1, 2)))
> > +           return false;
> > +       }
> > +      if (step != TREE_OPERAND (ref1, 3))
> > +       {
> > +         if (!step || !TREE_OPERAND (ref1, 3))
> > +           return false;
> > +         if (!operand_equal_p (step, TREE_OPERAND (ref1, 3)))
> > +           return false;
> > +       }
> > +      if (index2 != TREE_OPERAND (ref1, 4))
> > +       {
> > +         if (!index2 || !TREE_OPERAND (ref1, 4))
> > +           return false;
> > +         if (!operand_equal_p (index2, TREE_OPERAND (ref1, 4)))
> > +           return false;
> > +       }
> > +    }
>
> Ick!  I wonder if a safe_operand_equal_p that deals with NULL would help.  I'll
> note that using just == might actually work given while TMR_STEP can be
> constant it should be always of sth like sizetype for the base.

Yes it makes sense to have a safe_operand_equal_p; There is at least
one other location where this pattern shows up; mem_attrs_eq_p in
emit-rtl.cc :
```
          && (p->expr == q->expr
              || (p->expr != NULL_TREE && q->expr != NULL_TREE
                  && operand_equal_p (p->expr, q->expr, 0))));

```
mem_expr_equal_p, address_info_hasher::equal, perform_symbolic_merge are others.
I added it and will push a fixed up version after a bootstrap/test is completed.

>
> Otherwise OK.
>
> Thanks,
> Richard.
>
> > +
> > +  if (!types_compatible_p (TREE_TYPE (ref0), TREE_TYPE (ref1)))
> >      return false;
> >
> >    /* The alignment of the two accesses need to be the same.  */
> > @@ -4098,7 +4140,12 @@ factor_out_conditional_load (edge e0, edge e1, basic_block merge, gphi *phi,
> >
> >    /* Build the combined load RES = *PTR, reusing the PHI result so any range
> >       info on it is preserved (as factor_out_conditional_operation does).  */
> > -  tree nref = build2 (MEM_REF, TREE_TYPE (ref0), newptr, newindex);
> > +  tree nref;
> > +  if (index || step || index2)
> > +    nref = build5 (TARGET_MEM_REF, TREE_TYPE (ref0), newptr,
> > +                  newindex, index, step, index2);
> > +  else
> > +    nref = build2 (MEM_REF, TREE_TYPE (ref0), newptr, newindex);
> >    MR_DEPENDENCE_CLIQUE (nref) = clique;
> >    MR_DEPENDENCE_BASE (nref) = base;
> >    tree res = gimple_phi_result (phi);
> > --
> > 2.43.0
> >