Re: [PATCH v3] EXPR: Emit an truncate if 31+ bits polluted for SImode

[YunQiang Su <syq@gcc.gnu.org>]

Jeff Law <jeffreyalaw@gmail.com> 于2023年12月24日周日 00:51写道：
>
>
>
> On 12/23/23 01:58, YunQiang Su wrote:
> > On TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)) == true platforms,
> > if 31 or above bits is polluted by an bitops, we will need an
> > truncate. Let's emit one, and mark let's use the same hardreg
> > as in and out, the RTL may like:
> >
> > (insn 21 20 24 2 (set (subreg/s/u:SI (reg/v:DI 200 [ val ]) 0)
> >          (truncate:SI (reg/v:DI 200 [ val ]))) "../xx.c":7:29 -1
> >       (nil))
> >
> > We use /s/u flags to mark it as really needed, as in
> > combine_simplify_rtx, this insn may be considered as truncated,
> > so let's skip this combination.
> >
> > gcc/ChangeLog:
> >          PR: 104914.
> >          * combine.cc (try_combine): Skip combine with truncate if
> >       dest is subreg and has /u/s flags on platforms
> >       TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)) == true.
> >       * expr.cc (expand_assignment): Emit a truncate insn, if
> >       31+ bits is polluted for SImode.
> >
> > gcc/testsuite/ChangeLog:
> >       PR: 104914.
> >       * gcc.target/mips/pr104914.c: New testcase.
> I would suggest you show the RTL before/after whatever transformation
> has caused problems on your target and explain why you think the
> transformation is incorrect.
>

Before this patch, the RTL is like this
     (insn 19 18 20 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
               (const_int 8 [0x8])
               (const_int 24 [0x18]))
           (subreg:DI (reg:QI 205) 0)) "../xx.c":7:29 -1
        (nil))
      (insn 20 19 23 2 (set (reg/v:DI 200 [ val ])
           (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val ]) 0)))
"../xx.c":7:29 -1
       (nil))
     (jump_insn 23 20 24 2 (set (pc)
           (if_then_else (lt (subreg/s/u:SI (reg/v:DI 200 [ val ]) 0)
                   (const_int 0 [0]))
                (label_ref 32)
               (pc))) "../xx.c":10:5 -1
       (int_list:REG_BR_PROB 440234148 (nil))
      -> 32)

and then, when combine
      (insn 20 19 23 2 (set (reg/v:DI 200 [ val ])
             (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val ]) 0)))
"../xx.c":7:29 -1
      (nil))
will be convert to
          (note 20 19 23 2 NOTE_INSN_DELETED)
MIPS claims TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)) == true
based on that the hard register is always sign-extended, but here
the hard register is polluted by zero_extract.

If we just patch combine.cc to make it not eat sign_extend, here,
sign_extend will still disappear in the later passes, due to mips define
sign_extend as "emit_note (NOTE_INSN_DELETED)".

So I tried to insert a new truncate RTX here,
    (insn 21 20 24 2 (set (reg/v:DI 200 [ val ])
             (truncate:SI (reg/v:DI 200 [ val ]))) "../xx.c":7:29 -1
         (nil))
This is the RTL for this C code
     int32_t fun (int64_t arg) {
          int32_t a = (int32_t) arg;
          return a;
     }
But, the `reload` pass will get an ICE. I haven't dig the real problem.
If the new RTX is
    (insn 21 20 24 2 (set (subreg/s/u:SI (reg/v:DI 200 [ val ]) 0)
           (truncate:SI (reg/v:DI 200 [ val ]))) "../xx.c":7:29 -1
       (nil))
`reload` pass will happily accept it, and then it is converted to
     # this instruction will be sure the reg is well sign extended.
     `sll $rN, $rN, 0`
hard instruction.

The problem is that simple-rtx (called by combine) will believe that
REG 200 has been truncated to SImode, as the dest has an
subreg:SI.

So, I use /s/u flags to tell combine don't do so.

> Focus on the RTL semantics as well as the target specific semantics
> because both are critically important here.
>
> I strongly suspect you're just papering over a problem elsewhere.
>

Yes. I also guess so.  Any new idea?
In the previous threads, you suggested that we can just insert an
truncate instruction just before the comparison.
It still have some problem:
     1. There may be no comparison just after the zero_extract,
         instead some normal calculation, such as add/sub.
         Then, the calculation will get a malformed register, and
         in the ISA document, it is claimed UNPREDICTABLE.
     2. Insert an RTX before every comparison will cause performance
         regression, since in the most case, it is not needed.
     3. Inserting an RTX before comparison still needs some dirty hack
         like this.

>
> > ---
> >   gcc/combine.cc                           | 23 +++++++++++++++++++++-
> >   gcc/expr.cc                              | 17 ++++++++++++++++
> >   gcc/testsuite/gcc.target/mips/pr104914.c | 25 ++++++++++++++++++++++++
> >   3 files changed, 64 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/gcc.target/mips/pr104914.c
> >
> > diff --git a/gcc/combine.cc b/gcc/combine.cc
> > index 1cda4dd57f2..04b9c414053 100644
> > --- a/gcc/combine.cc
> > +++ b/gcc/combine.cc
> > @@ -3294,6 +3294,28 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
> >         n_occurrences = 0;            /* `subst' counts here */
> >         subst_low_luid = DF_INSN_LUID (i2);
> >
> > +      /* Don't try to combine a TRUNCATE INSN, if it's DEST is SUBREG and has
> > +      FLAG /s/u.  We use these 2 flags to mark this INSN as really needed:
> > +      normally, it means that the bits of 31+ of this variable is polluted
> > +      by a bitops.  The reason of existing of case (subreg:SI (reg:DI)) is
> > +      that, the same hardreg may act as src and dest.  */
> > +      if (TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)
> > +       && INSN_P (i2))
> > +     {
> > +       rtx i2dest_o = SET_DEST (PATTERN (i2));
> > +       rtx i2src_o = SET_SRC (PATTERN (i2));
> > +       if (GET_CODE (i2dest_o) == SUBREG
> > +           && GET_MODE (i2dest_o) == SImode
> > +           && GET_MODE (SUBREG_REG (i2dest_o)) == DImode
> > +           && SUBREG_PROMOTED_VAR_P (i2dest_o)
> > +           && SUBREG_PROMOTED_GET (i2dest_o) == SRP_SIGNED
> > +           && GET_CODE (i2src_o) == TRUNCATE
> > +           && GET_MODE (i2src_o) == SImode
> > +           && rtx_equal_p (SUBREG_REG (i2dest_o), XEXP (i2src_o, 0))
> > +           )
> > +         return 0;
> > +     }
> So checking SI/DI like this is just wrong.  There's nothing special
> about SI/DI.    Checking for equality between the destination and source

With some test, when the case of 8->32, 8->64, and 16->64, 16->64
don't have this problem, as sign_extend to SI/DI from HI/QI is not
NOOP, such as this RTX
    (insn 11 10 12 2 (set (reg/v:DI 198 [ val ])
           (sign_extend:DI (subreg:HI (reg/v:DI 198 [ val ]) 0))) "yy.c":4:29 -1
        (nil))
will be converted to:
      seh $rN, $rN

> also seems wrong -- if the state of the sign bit is wrong, it's wrong
> regardless of whether or not the source/destination register is the same.
>

I guess so, too.
In this patch, I try my best to be very careful not to break something else,
so I use very strict tests for this case.
The reason, I use this rtx_equal_p, based on that in expr.cc (see bellow),
I use
          emit_unop_insn (icode, to_rtx,
                     SUBREG_REG (to_rtx), TRUNCATE);

And in fact, if source/dest are different, the subreg in dest won't be needed,
In this case, combine won't eat this truncate insn.

I try my best to reduce the influence.

>
> >
> > @@ -5326,7 +5348,6 @@ find_split_point (rtx *loc, rtx_insn *insn, bool set_src)
> >
> >      UNIQUE_COPY is true if each substitution must be unique.  We do this
> >      by copying if `n_occurrences' is nonzero.  */
> > -
> >   static rtx
> >   subst (rtx x, rtx from, rtx to, bool in_dest, bool in_cond, bool unique_copy)
> >   {
> > diff --git a/gcc/expr.cc b/gcc/expr.cc
> > index 9fef2bf6585..f7236040a34 100644
> > --- a/gcc/expr.cc
> > +++ b/gcc/expr.cc
> > @@ -6284,6 +6284,23 @@ expand_assignment (tree to, tree from, bool nontemporal)
> >                                       nontemporal, reversep);
> >                 convert_move (SUBREG_REG (to_rtx), to_rtx1,
> >                               SUBREG_PROMOTED_SIGN (to_rtx));
> > +
> > +               rtx last = get_last_insn ();
> > +               if (TRULY_NOOP_TRUNCATION_MODES_P (DImode, SImode)
> > +                   && known_ge (bitregion_end, 31)
> > +                   && SUBREG_PROMOTED_VAR_P (to_rtx)
> > +                   && SUBREG_PROMOTED_SIGN (to_rtx) == SRP_SIGNED
> > +                   && GET_MODE (to_rtx) == SImode
> > +                   && GET_MODE (SUBREG_REG (to_rtx)) == DImode
> > +                   && GET_CODE (SET_SRC (PATTERN (last))) == SIGN_EXTEND
> > +                   )
> > +                 {
> > +                   insn_code icode = convert_optab_handler
> > +                                             (trunc_optab, SImode, DImode);
> > +                   if (icode != CODE_FOR_nothing)
> > +                     emit_unop_insn (icode, to_rtx,
> > +                                     SUBREG_REG (to_rtx), TRUNCATE);
> > +                 }
> Similar comments about the modes apply here.
>
> But again, my sense is there's a higher level problem here and that
> these changes are just papering over it.
>
> Jeff