[Bug target/104914] New: [MIPS] wrong comparison with scrabbled int value

[Bug target/104914] New: [MIPS] wrong comparison with scrabbled int value

[mmyangfl at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

            Bug ID: 104914
           Summary: [MIPS] wrong comparison with scrabbled int value
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: mmyangfl at gmail dot com
  Target Milestone: ---

GCC 12.0 (current git master, 80fcc4b) and 11 generates wrong instructions for
this code. (older version not tested)

$ mips64el-img-elf-gcc -mabi=64 -S -O1 -o - ~/a.c

#include <stdio.h>
void test(const unsigned char *buf) {
  int val;
  ((unsigned char*)&val)[0] = *buf++;
  ((unsigned char*)&val)[1] = *buf++;
  ((unsigned char*)&val)[2] = *buf++;
  ((unsigned char*)&val)[3] = *buf++;
  if(val > 0)
    puts("a");
  else
    fputs("b", stderr);
}

int main() { test("\xff\xff\xff\xff"); }  // => "a"

Generated asm code in question:

test:
        .frame  $sp,16,$31              # vars= 0, regs= 1/0, args= 0, gp= 0
        .mask   0x80000000,-8
        .fmask  0x00000000,0
        .set    noreorder
        .set    nomacro
        daddiu  $sp,$sp,-16
        sd      $31,8($sp)
        lbu     $3,0($4)
        move    $2,$0
        dins    $2,$3,0,8
        lbu     $3,1($4)
        dins    $2,$3,8,8
        lbu     $3,2($4)
        dins    $2,$3,16,8
        lbu     $3,3($4)
        dins    $2,$3,24,8
        blezc   $2,.L2             // signed extending $2 missing!
        lui     $4,%highest(.LC0)
        lui     $2,%hi(.LC0)
        daddiu  $4,$4,%higher(.LC0)
        daddiu  $2,$2,%lo(.LC0)
        dsll    $4,$4,32
        daddu   $4,$4,$2
        balc    puts
        ld      $31,8($sp)
.L5:
        daddiu  $sp,$sp,16
        jrc     $31
.L2:
        ld      $2,%gp_rel(_impure_ptr)($28)
        ld      $5,24($2)
        li      $4,98                   # 0x62
        balc    fputc
        b       .L5
        ld      $31,8($sp)

Below are my attempts to fix this bug:

-fdump-final-insns gives the following statement:

(jump_insn # 0 0 (set (pc)
        (if_then_else (le (reg:SI 2 $2 [orig:201 val ] [201])
                (const_int 0 [0]))
            (label_ref #)
            (pc))) "/home/ding/a.c":8:5# {*branch_ordersi}
     (expr_list:REG_DEAD (reg:SI 2 $2 [orig:201 val ] [201])
        (int_list:REG_BR_PROB 440234148 (nil)))
 -> 2)

After manually `icode != CODE_FOR_cbranchsi4` in gcc/gcc/optabs.cc:4501,
combine pass still combines them back, but the machine description simply
define "cbranch<mode>4" for all cbranch family.

I wonder since MIPS64 can't really do comparsion over partial register, is this
RTL valid?

[Bug target/104914] [MIPS] wrong comparison with scrabbled int value

[mmyangfl at gmail dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #1 from Yangfl <mmyangfl at gmail dot com> ---
Original issue: https://github.com/matplotlib/matplotlib/issues/21789

[Bug target/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

YunQiang Su <syq at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |syq at gcc dot gnu.org

--- Comment #2 from YunQiang Su <syq at gcc dot gnu.org> ---
In fact, gcc-11 works with `short', for which, seh is used to sign extending.

[Bug target/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #3 from YunQiang Su <syq at gcc dot gnu.org> ---
Ohhh, I think that the real problem is that:

    why DINS is used instead of INS when work with an INT?

[Bug target/104914] [MIPS] wrong comparison with scrabbled int value

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |pinskia at gcc dot gnu.org

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I had remembered fixing a similar issue before. I will look at this later
today. And yes ins should have been used instead of dins.

[Bug target/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #5 from YunQiang Su <syq at gcc dot gnu.org> ---
Another possible fix is to emit a SLL for extendsidi2, but
I am worrying about it may break something else.

[Bug target/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #6 from YunQiang Su <syq at gcc dot gnu.org> ---
In RTL (xx.c.256r.expand), there is 

(insn 10 9 11 2 (set (zero_extract:DI (reg/v:DI 200 [ val ])
            (const_int 8 [0x8])
            (const_int 0 [0]))
        (subreg:DI (reg:QI 202) 0)) "xx.c":4:29 -1
     (nil))


Maybe, it wrong as the mode cannot show the real length of val.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
          Component|target                      |rtl-optimization
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-07-06
             Status|UNCONFIRMED                 |NEW
      Known to fail|                            |11.2.0

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
The initial RTL has a signed extend in there:


(insn 20 19 23 2 (set (reg/v:DI 200 [ val+-4 ])
        (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val+-4 ]) 4)))
"/app/example.cpp":7:29 -1
     (nil))
(jump_insn 23 20 24 2 (set (pc)
        (if_then_else (le (subreg/s/u:SI (reg/v:DI 200 [ val+-4 ]) 4)
                (const_int 0 [0]))
            (label_ref 32)
            (pc))) "/app/example.cpp":8:5 -1
     (int_list:REG_BR_PROB 440234148 (nil))
 -> 32)


Before combine also looks fine:
(insn 20 19 23 2 (set (reg/v:DI 200 [ val+-4 ])
        (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val+-4 ]) 4)))
"/app/example.cpp":7:29 235 {extendsidi2}
     (nil))
(jump_insn 23 20 24 2 (set (pc)
        (if_then_else (le (subreg/s/u:SI (reg/v:DI 200 [ val+-4 ]) 4)
                (const_int 0 [0]))
            (label_ref 32)
            (pc))) "/app/example.cpp":8:5 471 {*branch_ordersi}
     (expr_list:REG_DEAD (reg/v:DI 200 [ val+-4 ])
        (int_list:REG_BR_PROB 440234148 (nil)))
 -> 32)

But combine does the wrong thing:
Trying 20 -> 23:
   20: r200:DI=sign_extend(r200:DI#4)
   23: pc={(r200:DI#4<=0)?L32:pc}
      REG_DEAD r200:DI
      REG_BR_PROB 440234148
Successfully matched this instruction:
(set (pc)
    (if_then_else (le (subreg:SI (reg/v:DI 200 [ valD.1959+-4 ]) 4)
            (const_int 0 [0]))
        (label_ref 32)
        (pc)))
allowing combination of insns 20 and 23
original costs 4 + 16 = 20
replacement cost 16
deferring deletion of insn with uid = 20.
modifying insn i3    23: pc={(r200:DI#4<=0)?L32:pc}
      REG_BR_PROB 440234148
      REG_DEAD r200:DI
deferring rescan insn with uid = 23.

Instead of a subreg here, it should have been a truncate.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #8 from YunQiang Su <syq at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #7)
> The initial RTL has a signed extend in there:
> 
> 
> (insn 20 19 23 2 (set (reg/v:DI 200 [ val+-4 ])
>         (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val+-4 ]) 4)))
> "/app/example.cpp":7:29 -1
>      (nil))
> (jump_insn 23 20 24 2 (set (pc)
>         (if_then_else (le (subreg/s/u:SI (reg/v:DI 200 [ val+-4 ]) 4)
>                 (const_int 0 [0]))
>             (label_ref 32)
>             (pc))) "/app/example.cpp":8:5 -1
>      (int_list:REG_BR_PROB 440234148 (nil))
>  -> 32)
> 
> 
> Before combine also looks fine:
> (insn 20 19 23 2 (set (reg/v:DI 200 [ val+-4 ])
>         (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val+-4 ]) 4)))
> "/app/example.cpp":7:29 235 {extendsidi2}
>      (nil))

Yes. I noticed it. while in mips.md,  extendsidi2 is expanded to no
instructions at all.




```
;; Extension insns.
;; Those for integer source operand are ordered widest source type first.

;; When TARGET_64BIT, all SImode integer and accumulator registers
;; should already be in sign-extended form (see TARGET_TRULY_NOOP_TRUNCATION
;; and truncdisi2).  We can therefore get rid of register->register
;; instructions if we constrain the source to be in the same register as
;; the destination.
;;
;; Only the pre-reload scheduler sees the type of the register alternatives;
;; we split them into nothing before the post-reload scheduler runs.
;; These alternatives therefore have type "move" in order to reflect
;; what happens if the two pre-reload operands cannot be tied, and are
;; instead allocated two separate GPRs.  We don't distinguish between
;; the GPR and LO cases because we don't usually know during pre-reload
;; scheduling whether an operand will be LO or not.
(define_insn_and_split "extendsidi2"
  [(set (match_operand:DI 0 "register_operand" "=d,l,d")
        (sign_extend:DI (match_operand:SI 1 "nonimmediate_operand" "0,0,m")))]
  "TARGET_64BIT"
  "@
   #
   #
   lw\t%0,%1"
  "&& reload_completed && register_operand (operands[1], VOIDmode)"
  [(const_int 0)]
{
  emit_note (NOTE_INSN_DELETED);
  DONE;
}
  [(set_attr "move_type" "move,move,load")
   (set_attr "mode" "DI")])
```

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           See Also|                            |https://gcc.gnu.org/bugzill
                   |                            |a/show_bug.cgi?id=67736

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to YunQiang Su from comment #8)
> (In reply to Andrew Pinski from comment #7)
> > The initial RTL has a signed extend in there:
> > 
> > 
> > (insn 20 19 23 2 (set (reg/v:DI 200 [ val+-4 ])
> >         (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val+-4 ]) 4)))
> > "/app/example.cpp":7:29 -1
> >      (nil))
> > (jump_insn 23 20 24 2 (set (pc)
> >         (if_then_else (le (subreg/s/u:SI (reg/v:DI 200 [ val+-4 ]) 4)
> >                 (const_int 0 [0]))
> >             (label_ref 32)
> >             (pc))) "/app/example.cpp":8:5 -1
> >      (int_list:REG_BR_PROB 440234148 (nil))
> >  -> 32)
> > 
> > 
> > Before combine also looks fine:
> > (insn 20 19 23 2 (set (reg/v:DI 200 [ val+-4 ])
> >         (sign_extend:DI (subreg:SI (reg/v:DI 200 [ val+-4 ]) 4)))
> > "/app/example.cpp":7:29 235 {extendsidi2}
> >      (nil))
> 
> Yes. I noticed it. while in mips.md,  extendsidi2 is expanded to no
> instructions at all.

Right then the le should had a truncation before the use of SI mode here ...

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #10 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 55496
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=55496&action=edit
old patch against GCC 4.7

I am trying to find my notes on this old patch but our internal bug system has
moved a few times and the project looks archived even.
But I am pretty sure this is related to the problem at hand.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #10)
> Created attachment 55496 [details]
> old patch against GCC 4.7
> 
> I am trying to find my notes on this old patch but our internal bug system
> has moved a few times and the project looks archived even.
> But I am pretty sure this is related to the problem at hand.

(note I had another patch before that which renamed store_bit_field_1 to
store_bit_field_2).

The code is now in store_bit_field_using_insv.
Here:
          else
            {
              tmp = gen_lowpart_if_possible (op_mode, value1);
              if (! tmp)
                tmp = gen_lowpart (op_mode, force_reg (value_mode, value1));
            }
          value1 = tmp;
        }

But I don't have any other notes on my change (nor a testcase).

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #12 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #11)
> But I don't have any other notes on my change (nor a testcase).

So I found some notes and it is similar but still different.
We were expanding:
;; insn.j_format.target = D.21597_19;
Into
(insn 25 24 26 (set (reg:DI 220)
        (lshiftrt:DI (reg:DI 196 [ D.21584 ])
            (const_int 2 [0x2]))) arch/mips/kernel/jump_label.c:56 -1
     (nil))

(insn 26 25 0 (set (zero_extract:SI (reg/v:SI 208 [ insn ])
            (const_int 26 [0x1a])
            (const_int 6 [0x6]))
        (subreg:SI (reg:DI 220) 4)) arch/mips/kernel/jump_label.c:56 -1
     (nil))

But the subreg there was incorrect.

In this case of this bug, the reg is DI rather than SI. I wonder why we have
that in the first place even though val is the size of SImode ...

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #13 from YunQiang Su <syq at gcc dot gnu.org> ---
This tiny patch works will this small test case by replace with dins to ins.

I have no idea whether it will have any side effects.

Any idea?

diff --git a/gcc/expmed.cc b/gcc/expmed.cc
index fbd4ce2d42f..37f90912122 100644
--- a/gcc/expmed.cc
+++ b/gcc/expmed.cc
@@ -849,7 +849,7 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
poly_uint64 bitnum,
      if we aren't.  This must come after the entire register case above,
      since that case is valid for any mode.  The following cases are only
      valid for integral modes.  */
-  opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
+  opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (str_rtx));
   scalar_int_mode imode;
   if (!op0_mode.exists (&imode) || imode != GET_MODE (op0))
     {

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #14 from YunQiang Su <syq at gcc dot gnu.org> ---
New patch:

diff --git a/gcc/expmed.cc b/gcc/expmed.cc
index fbd4ce2d42f..66d45da67df 100644
--- a/gcc/expmed.cc
+++ b/gcc/expmed.cc
@@ -850,6 +850,7 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
poly_uint64 bitnum,
      since that case is valid for any mode.  The following cases are only
      valid for integral modes.  */
   opt_scalar_int_mode op0_mode = int_mode_for_mode (GET_MODE (op0));
+  opt_scalar_int_mode str_mode = int_mode_for_mode (GET_MODE (str_rtx));
   scalar_int_mode imode;
   if (!op0_mode.exists (&imode) || imode != GET_MODE (op0))
     {
@@ -881,8 +882,15 @@ store_bit_field_1 (rtx str_rtx, poly_uint64 bitsize,
poly_uint64 bitnum,
        op0 = gen_lowpart (op0_mode.require (), op0);
     }

-  return store_integral_bit_field (op0, op0_mode, ibitsize, ibitnum,
-                                  bitregion_start, bitregion_end,
+  bool use_str_mode = false;
+  if (GET_MODE_CLASS(GET_MODE (str_rtx)) == MODE_INT
+      && GET_MODE_CLASS(GET_MODE (op0)) == MODE_INT
+      && known_gt (GET_MODE_SIZE(GET_MODE(op0)),
GET_MODE_SIZE(GET_MODE(str_rtx))))
+    use_str_mode = true;
+
+  return store_integral_bit_field (use_str_mode ? str_rtx : op0,
+                                  use_str_mode ? str_mode : op0_mode,
+                                  ibitsize, ibitnum, bitregion_start,
bitregion_end,
                                   fieldmode, value, reverse, fallback_p);
 }

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[roger at nextmovesoftware dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

Roger Sayle <roger at nextmovesoftware dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |roger at nextmovesoftware dot com

--- Comment #15 from Roger Sayle <roger at nextmovesoftware dot com> ---
Is MIPS64 actually a TRULY_NOOP_TRUNCATION_TARGET?  If SImode is implicitly
assumed to be (sign?) extended, then an arbitrary DImode value/register can't
be used as an SImode value without appropriately setting/clearing the upper
bits.
i.e. thus this integer truncation isn't a no-op.

I suspect that the underlying problem is that the backend is relying on
implicit invariants, not explicitly represented in the RTL, and then surprised
when valid RTL transformations don't preserve those invariants/assumptions.

I wonder why the zero_extract followed by sign_extend example mentioned in
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626137.html isn't already
being considered as a try_combine candidate, allowing the backend to simply
recognize or split it.  I'll investigate.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #16 from YunQiang Su <syq at gcc dot gnu.org> ---
(In reply to Roger Sayle from comment #15)
> Is MIPS64 actually a TRULY_NOOP_TRUNCATION_TARGET?  If SImode is implicitly
> assumed to be (sign?) extended, then an arbitrary DImode value/register
> can't be used as an SImode value without appropriately setting/clearing the
> upper bits.
> i.e. thus this integer truncation isn't a no-op.
> 

in gcc/config/mips/mips.cc, there are lines:

static bool
mips_truly_noop_truncation (poly_uint64 outprec, poly_uint64 inprec)
{
  return !TARGET_64BIT || inprec <= 32 || outprec > 32;
}


So for mips_truly_noop_truncation(64, 32), it is true, aka we can convert
32bit value to 64bit value without any insn.

This setting is based on that most (if not all) word (32bit) operation insns
are all sign-extend.

For example, when we run these instructions on a MIPS64 CPU
      li $a1, 0x7fffffff
      add $a3, $a1, $a1
The result of $a3 will be:
      0xffffffff fffffffe

And for theses instructions:
      li $a1, 0x7fffffff
      dadd $a3, $a1, $a1  # note, add -> dadd
Then the content of $a3 will be:
     0x00000000 fffffffe


And MIPS has the single instruction for: branch less than zero, for both
MIPS32, MIPS64.

Let me explain example 1:
    if the code is running on a 32bit CPU, the result of $a3 will be
0xfffffffe, which is -2.
    if the code is running on a 64bit CPU, since the result of $a3 will be
sign-extend to 0xffffffff fffffffe,
       it is still -2.

That's how MIPS make 32bit binaries run smoothly on a 64bit CPU 
without any mode switch.

> I suspect that the underlying problem is that the backend is relying on
> implicit invariants, not explicitly represented in the RTL, and then
> surprised when valid RTL transformations don't preserve those
> invariants/assumptions.
> 
> I wonder why the zero_extract followed by sign_extend example mentioned in
> https://gcc.gnu.org/pipermail/gcc-patches/2023-August/626137.html isn't
> already being considered as a try_combine candidate, allowing the backend to
> simply recognize or split it.  I'll investigate.

Thanks.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[roger at nextmovesoftware dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #17 from Roger Sayle <roger at nextmovesoftware dot com> ---
I think this patch might resolve the problem (or move it somewhere else):

diff --git a/gcc/expr.cc b/gcc/expr.cc
index 9fef2bf6585..218bca905f5 100644
--- a/gcc/expr.cc
+++ b/gcc/expr.cc
@@ -6274,10 +6274,7 @@ expand_assignment (tree to, tree from, bool nontemporal)
                result = store_expr (from, to_rtx, 0, nontemporal, false);
              else
                {
-                 rtx to_rtx1
-                   = lowpart_subreg (subreg_unpromoted_mode (to_rtx),
-                                     SUBREG_REG (to_rtx),
-                                     subreg_promoted_mode (to_rtx));
+                 rtx to_rtx1 = gen_reg_rtx (subreg_unpromoted_mode (to_rtx));
                  result = store_field (to_rtx1, bitsize, bitpos,
                                        bitregion_start, bitregion_end,
                                        mode1, from, get_alias_set (to),

The motivation/solution comes from a comment in expmed.cc:
/* If the destination is a paradoxical subreg such that we need a
   truncate to the inner mode, perform the insertion on a temporary and
   truncate the result to the original destination.  Note that we can't
   just truncate the paradoxical subreg as (truncate:N (subreg:W (reg:N
   X) 0)) is (reg:N X).  */

The same caveat applies to extensions on MIPS, so we should use a new
pseudo temporary register rather than update the SUBREG in place.

If someone could confirm this fixes the issue on MIPS, I'll try to come up
with a milder form of this fix that checks TARGET_MODE_REP_EXTENDED that'll
limit the churn/impact on other targets.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[roger at nextmovesoftware dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #18 from Roger Sayle <roger at nextmovesoftware dot com> ---
Please ignore comment #17, the above patch is completely bogus/broken.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[roger at nextmovesoftware dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #19 from Roger Sayle <roger at nextmovesoftware dot com> ---
Created attachment 56930
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56930&action=edit
proposed patch

And now for a patch that does (or should) work.  This even contains an
optimization, we middle-end knows we don't need to sign or zero extend if a
insv doesn't modify the sign-bit.  Testing on MIPS would be much appreciated. 
TIA.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #20 from YunQiang Su <syq at gcc dot gnu.org> ---
This patch has 2 problems:

1. We may need some more steps to add
   gcc_assert (outprec < inprec)
   Now, I met some ICE with it.

2. It doesn't solve the this problem:
   In combine.cc, jump_insn eats truncate and sign_extend.
   In fact that is the real problem: How to tell combine.cc:
       don't eat it; this truncate/sign_extend is really needed by us.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #21 from YunQiang Su <syq at gcc dot gnu.org> ---
Sorry, Roger. Your patch is correct.
I misunderstood it.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #22 from YunQiang Su <syq at gcc dot gnu.org> ---
Any way, we should split the assert to another patch.

I will try to find all the wrongly used TRULY_NOOP_TRUNCATION_MODES_P.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #23 from YunQiang Su <syq at gcc dot gnu.org> ---
I guess, we should drop TRULY_NOOP_TRUNCATION_MODES_P and
TARGET_MODE_REP_EXTENDED for MIPS. In fact, it will only effect MIPS64 SI->DI.

Then it may reduce the maintain workload for MIPS64.

Let's have a try and run some benchmark.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #24 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by YunQiang Su <syq@gcc.gnu.org>:

https://gcc.gnu.org/g:65d4b32dee2508f5bcdd999a132332cd46cf8a4d

commit r14-6905-g65d4b32dee2508f5bcdd999a132332cd46cf8a4d
Author: YunQiang Su <syq@gcc.gnu.org>
Date:   Sat Dec 30 00:17:52 2023 +0800

    MIPS: Add pattern insqisi_extended and inshisi_extended

    This match pattern allows combination (zero_extract:DI 8, 24, QI)
    with an sign-extend to 32bit INS instruction on TARGET_64BIT.

    For SI mode, if the sign-bit is modified by bitops, we will need a
    sign-extend operation.  Since 32bit INS instruction can be sure that
    result is sign-extended, and the QImode src register is safe for INS, too.

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

    Combine try to merge them to:

    (insn 20 19 23 2 (set (reg/v:DI 200 [ val ])
            (sign_extend:DI (ior:SI (and:SI (subreg:SI (reg/v:DI 200 [ val ])
0)
                        (const_int 16777215 [0xffffff]))
                    (ashift:SI (subreg:SI (reg:QI 205 [ MEM[(const unsigned
char *)buf_8(D) + 3B] ]) 0)
                        (const_int 24 [0x18]))))) "../xx.c":7:29 18
{*insv_extended}
         (expr_list:REG_DEAD (reg:QI 205 [ MEM[(const unsigned char *)buf_8(D)
+ 3B] ])
            (nil)))

    And do similarly for 16/16 pair:
    (insn 13 12 14 2 (set (zero_extract:DI (reg/v:DI 198 [ val ])
                (const_int 16 [0x10])
                (const_int 16 [0x10]))
            (subreg:DI (reg:HI 201 [ MEM[(const short unsigned int *)buf_6(D) +
2B] ]) 0)) "xx.c":5:30 286 {*insvdi}
         (expr_list:REG_DEAD (reg:HI 201 [ MEM[(const short unsigned int
*)buf_6(D) + 2B] ])
            (nil)))
    (insn 14 13 17 2 (set (reg/v:DI 198 [ val ])
            (sign_extend:DI (subreg:SI (reg/v:DI 198 [ val ]) 0))) "xx.c":5:30
241 {extendsidi2}
         (nil))
    ------------>
    (insn 14 13 17 2 (set (reg/v:DI 198 [ val ])
            (sign_extend:DI (ior:SI (ashift:SI (subreg:SI (reg:HI 201 [
MEM[(const short unsigned int *)buf_6(D) + 2B] ]) 0)
                        (const_int 16 [0x10]))
                    (zero_extend:SI (subreg:HI (reg/v:DI 198 [ val ]) 0)))))
"xx.c":5:30 284 {*inshisi_extended}
         (expr_list:REG_DEAD (reg:HI 201 [ MEM[(const short unsigned int
*)buf_6(D) + 2B] ])
            (nil)))

    Let's accept these patterns, and set the cost to 1 instruction.

    gcc

            PR rtl-optimization/104914
            * config/mips/mips.md (insqisi_extended): New patterns.
            (inshisi_extended): Ditto.

    gcc/testsuite

            * gcc.target/mips/pr104914.c: New test.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

--- Comment #25 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Roger Sayle <sayle@gcc.gnu.org>:

https://gcc.gnu.org/g:3ac58063114cf491891072be6205d32a42c6707d

commit r14-6915-g3ac58063114cf491891072be6205d32a42c6707d
Author: Roger Sayle <roger@nextmovesoftware.com>
Date:   Thu Jan 4 10:49:33 2024 +0000

    Improved RTL expansion of field assignments into promoted registers.

    This patch fixes PR rtl-optmization/104914 by tweaking/improving the way
    the fields are written into a pseudo register that needs to be kept sign
    extended.

    The motivating example from the bugzilla PR is:

    extern void ext(int);
    void foo(const unsigned char *buf) {
      int val;
      ((unsigned char*)&val)[0] = *buf++;
      ((unsigned char*)&val)[1] = *buf++;
      ((unsigned char*)&val)[2] = *buf++;
      ((unsigned char*)&val)[3] = *buf++;
      if(val > 0)
        ext(1);
      else
        ext(0);
    }

    which at the end of the tree optimization passes looks like:

    void foo (const unsigned char * buf)
    {
      int val;
      unsigned char _1;
      unsigned char _2;
      unsigned char _3;
      unsigned char _4;
      int val.5_5;

      <bb 2> [local count: 1073741824]:
      _1 = *buf_7(D);
      MEM[(unsigned char *)&val] = _1;
      _2 = MEM[(const unsigned char *)buf_7(D) + 1B];
      MEM[(unsigned char *)&val + 1B] = _2;
      _3 = MEM[(const unsigned char *)buf_7(D) + 2B];
      MEM[(unsigned char *)&val + 2B] = _3;
      _4 = MEM[(const unsigned char *)buf_7(D) + 3B];
      MEM[(unsigned char *)&val + 3B] = _4;
      val.5_5 = val;
      if (val.5_5 > 0)
        goto <bb 3>; [59.00%]
      else
        goto <bb 4>; [41.00%]

      <bb 3> [local count: 633507681]:
      ext (1);
      goto <bb 5>; [100.00%]

      <bb 4> [local count: 440234144]:
      ext (0);

      <bb 5> [local count: 1073741824]:
      val ={v} {CLOBBER(eol)};
      return;

    }

    Here four bytes are being sequentially written into the SImode value
    val.  On some platforms, such as MIPS64, this SImode value is kept in
    a 64-bit register, suitably sign-extended.  The function expand_assignment
    contains logic to handle this via SUBREG_PROMOTED_VAR_P (around line 6264
    in expr.cc) which outputs an explicit extension operation after each
    store_field (typically insv) to such promoted/extended pseudos.

    The first observation is that there's no need to perform sign extension
    after each byte in the example above; the extension is only required
    after changes to the most significant byte (i.e. to a field that overlaps
    the most significant bit).

    The bug fix is actually a bit more subtle, but at this point during
    code expansion it's not safe to use a SUBREG when sign-extending this
    field.  Currently, GCC generates (sign_extend:DI (subreg:SI (reg:DI) 0))
    but combine (and other RTL optimizers) later realize that because SImode
    values are always sign-extended in their 64-bit hard registers that
    this is a no-op and eliminates it.  The trouble is that it's unsafe to
    refer to the SImode lowpart of a 64-bit register using SUBREG at those
    critical points when temporarily the value isn't correctly sign-extended,
    and the usual backend invariants don't hold.  At these critical points,
    the middle-end needs to use an explicit TRUNCATE rtx (as this isn't a
    TRULY_NOOP_TRUNCATION), so that the explicit sign-extension looks like
    (sign_extend:DI (truncate:SI (reg:DI)), which avoids the problem.

    2024-01-04  Roger Sayle  <roger@nextmovesoftware.com>
                Jeff Law  <jlaw@ventanamicro.com>

    gcc/ChangeLog
            PR rtl-optimization/104914
            * expr.cc (expand_assignment): When target is SUBREG_PROMOTED_VAR_P
            a sign or zero extension is only required if the modified field
            overlaps the SUBREG's most significant bit.  On MODE_REP_EXTENDED
            targets, don't refer to the temporarily incorrectly extended value
            using a SUBREG, but instead generate an explicit TRUNCATE rtx.

[Bug rtl-optimization/104914] [MIPS] wrong comparison with scrabbled int value

[syq at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104914

YunQiang Su <syq at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|NEW                         |RESOLVED

--- Comment #26 from YunQiang Su <syq at gcc dot gnu.org> ---
Since we have 2 fixes both fixed this problem.
Let's close it.

Should we back port it to gcc13/gcc12?