[Bug target/113010] New: [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[Bug target/113010] New: [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[gkm at rivosinc dot com]

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

            Bug ID: 113010
           Summary: [RISCV] sign-extension lost in comparison with
                    constant embedded in comma-op expression
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: gkm at rivosinc dot com
  Target Milestone: ---

Implicit conversion rules in C for comparing (unsigned long) with (int) require
that (int) first widen to (long) extending sign, then convert to (unsigned
long). 

I debugged this far enough to see that expand_compound_operation () in
combine.cc converts (SIGN_EXTEND:DI (mem/c:SI ... )) into paradoxical
(SUBREG:DI (mem/c:SI ... )), and thus loses the sign extension.

It only happens when the constant comparand is within a comma operator.


Test case:
--------------------------------------------------
int minus_1 = -1;

int main ()
{
  return ((0, 0xfffffffful) < minus_1) ? 0 : 1;
}
--------------------------------------------------
Godbolt:
https://godbolt.org/z/xPMPzG1n1

The -march=rv64gc case begins to fail with gcc-13
The -march=rv64gc_zbs case begins to fail with gcc-12

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |DUPLICATE

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Dup of bug 112758.  See the discussion there ...

*** This bug has been marked as a duplicate of bug 112758 ***

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[patrick at rivosinc dot com]

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

--- Comment #2 from Patrick O'Neill <patrick at rivosinc dot com> ---
Andrew I don't think this is a duplicate of pr112758 (or at least it wasn't
resolved by the fix for pr112758).

I still see the behavior on r14-6902-g4a0a8dc1b88.

> /scratch/tc-testing/tc-jan-3-trunk/build-rv64gcv/bin/riscv64-unknown-linux-gnu-gcc -O2 -march=rv64gc red.c -o user-config.out                                                                           
> QEMU_CPU=rv64,zbs=true timeout --verbose -k 0.1 1 /scratch/tc-testing/tc-jan-3-trunk/build-rv64gcv/bin/qemu-riscv64 user-config.out
> echo $?                                                                                            1

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[gkm at rivosinc dot com]

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

Greg McGary <gkm at rivosinc dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|DUPLICATE                   |---

--- Comment #3 from Greg McGary <gkm at rivosinc dot com> ---
This fixes it, though I would like second look from someone more familiar with
the combiner:

diff --git a/gcc/combine.cc b/gcc/combine.cc
index 812553c091e..cd159db2a33 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -7208,6 +7208,10 @@ expand_compound_operation (rtx x)
       if (len == 0)
        return x;

+      /* Sign-extending loads can never be simplified at compile time.  */
+      if (MEM_P (XEXP (x, 0)) && load_extend_op (inner_mode) == SIGN_EXTEND)
+ return x;
+
       break;

     case ZERO_EXTRACT:

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[pinskia at gcc dot gnu.org]

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

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Greg McGary from comment #3)
> This fixes it, though I would like second look from someone more familiar
> with the combiner:

I almost sure this is still an issue with WORD_REGISTER_OPERATIONS .

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2024-01-09
   Target Milestone|---                         |13.3

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #4)
> (In reply to Greg McGary from comment #3)
> > This fixes it, though I would like second look from someone more familiar
> > with the combiner:
> 
> I almost sure this is still an issue with WORD_REGISTER_OPERATIONS .

That is this is missing that check:

```
  /* Convert sign extension to zero extension, if we know that the high
     bit is not set, as this is easier to optimize.  It will be converted
     back to cheaper alternative in make_extraction.  */
  if (GET_CODE (x) == SIGN_EXTEND
      && HWI_COMPUTABLE_MODE_P (mode)
      && ((nonzero_bits (XEXP (x, 0), inner_mode)
           & ~(((unsigned HOST_WIDE_INT) GET_MODE_MASK (inner_mode)) >> 1))
          == 0))

```

Should most likely need the same check as what was added in r14-6806 . And yes
it a similar bug dealing with WORD_REGISTER_OPERATIONS  which is why I thought
it was the same.

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|13.3                        |---

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[gkm at rivosinc dot com]

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

--- Comment #6 from Greg McGary <gkm at rivosinc dot com> ---
(In reply to Andrew Pinski from comment #5)
> (In reply to Andrew Pinski from comment #4)
> > (In reply to Greg McGary from comment #3)
> > > This fixes it, though I would like second look from someone more familiar
> > > with the combiner:
> > 
> > I almost sure this is still an issue with WORD_REGISTER_OPERATIONS .
> 
> That is this is missing that check:
> 
> ```
>   /* Convert sign extension to zero extension, if we know that the high
>      bit is not set, as this is easier to optimize.  It will be converted
>      back to cheaper alternative in make_extraction.  */
>   if (GET_CODE (x) == SIGN_EXTEND
>       && HWI_COMPUTABLE_MODE_P (mode)
>       && ((nonzero_bits (XEXP (x, 0), inner_mode)
>            & ~(((unsigned HOST_WIDE_INT) GET_MODE_MASK (inner_mode)) >> 1))
>           == 0))
> 
> ```
> 
> Should most likely need the same check as what was added in r14-6806 . And
> yes it a similar bug dealing with WORD_REGISTER_OPERATIONS  which is why I
> thought it was the same.

I agree that it pertains to WORD_REGISTER_OPERATIONS. However, the check needs
to happen. The test for converting SIGN_EXTEND to ZERO_EXTEND is false for
MEM_P (x) already. The erroneous conversion to SUBREG happens farther down,
here:

```
  modewidth = GET_MODE_PRECISION (mode);
  if (modewidth >= pos + len)
    {
=>    tem = gen_lowpart (mode, XEXP (x, 0));
      if (!tem || GET_CODE (tem) == CLOBBER)
        return x;
      tem = simplify_shift_const (NULL_RTX, ASHIFT, mode,
                                  tem, modewidth - pos - len);
      tem = simplify_shift_const (NULL_RTX, unsignedp ? LSHIFTRT : ASHIFTRT,
                                  mode, tem, modewidth - len);
    }
```

This fix honors WORD_REGISTER_OPERATIONS, and adds a test:

```
diff --git a/gcc/combine.cc b/gcc/combine.cc
index 812553c091e..ba587184dfc 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -7208,6 +7208,11 @@ expand_compound_operation (rtx x)
       if (len == 0)
        return x;

+      /* Sign-extending loads can never be simplified at compile time.  */
+      if (WORD_REGISTER_OPERATIONS && MEM_P (XEXP (x, 0))
+   && load_extend_op (inner_mode) == SIGN_EXTEND)
+ return x;
+
       break;

     case ZERO_EXTRACT:
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr113010.c
b/gcc/testsuite/gcc.c-torture/execute/pr113010.c
new file mode 100644
index 00000000000..a95c613c1df
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr113010.c
@@ -0,0 +1,9 @@
+int minus_1 = -1;
+
+int
+main ()
+{
+  if ((0, 0xfffffffful) >= minus_1)
+    __builtin_abort ();
+  return 0;
+}
```

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[gkm at rivosinc dot com]

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

--- Comment #7 from Greg McGary <gkm at rivosinc dot com> ---
(In reply to Greg McGary from comment #6)
> I agree that it pertains to WORD_REGISTER_OPERATIONS. However, the check
> needs to happen.

... needs to happen *earlier*.

(I wish comments were editable)

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[patrick at rivosinc dot com]

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

--- Comment #8 from Patrick O'Neill <patrick at rivosinc dot com> ---
From the CI run here:
https://patchwork.sourceware.org/project/gcc/patch/20240116221914.267015-1-gkm@rivosinc.com/

It looks like this issue also affects ARM since the testcase fails.

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[pinskia at gcc dot gnu.org]

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

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Patrick O'Neill from comment #8)
> From the CI run here:
> https://patchwork.sourceware.org/project/gcc/patch/20240116221914.267015-1-
> gkm@rivosinc.com/
> 
> It looks like this issue also affects ARM since the testcase fails.

That is aarch32 (a ILP32 target) where long is the same size as int. I suspect
you will end up with a failure even on i386 (32bit) with the current testcase.

I suspect you want to use either 0xffffffffull or limit it to `sizeof(long) >
sizeof(int)` targets.

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:24975a9195743e8eb4ca213f35b9221d4eeb6b59

commit r14-9284-g24975a9195743e8eb4ca213f35b9221d4eeb6b59
Author: Greg McGary <gkm@rivosinc.com>
Date:   Sun Mar 3 14:49:49 2024 -0700

    [PATCH] combine: Don't simplify paradoxical SUBREG on
WORD_REGISTER_OPERATIONS [PR113010]

    The sign-bit-copies of a sign-extending load cannot be known until runtime
on
    WORD_REGISTER_OPERATIONS targets, except in the case of a zero-extending
MEM
    load.  See the fix for PR112758.

    gcc/
            PR rtl-optimization/113010
            * combine.cc (simplify_comparison): Simplify a SUBREG on
            WORD_REGISTER_OPERATIONS targets only if it is a zero-extending
            MEM load.

    gcc/testsuite
            * gcc.c-torture/execute/pr113010.c: New test.

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[law at gcc dot gnu.org]

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

Jeffrey A. Law <law at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
                 CC|                            |law at gcc dot gnu.org
         Resolution|---                         |FIXED

--- Comment #11 from Jeffrey A. Law <law at gcc dot gnu.org> ---
Fixed by Greg's patch on the trunk.  No current plans to backport.

[Bug target/113010] [RISCV] sign-extension lost in comparison with constant embedded in comma-op expression

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jakub Jelinek <jakub@gcc.gnu.org>:

https://gcc.gnu.org/g:901e7bdab70e2275723ac31dacbbce0b6f68f4f4

commit r14-9304-g901e7bdab70e2275723ac31dacbbce0b6f68f4f4
Author: Jakub Jelinek <jakub@redhat.com>
Date:   Mon Mar 4 19:23:02 2024 +0100

    combine: Fix recent WORD_REGISTER_OPERATIONS check [PR113010]

    On Mon, Mar 04, 2024 at 05:18:39PM +0100, Rainer Orth wrote:
    > unfortunately, the patch broke Solaris/SPARC bootstrap
    > (sparc-sun-solaris2.11):
    >
    > .../gcc/combine.cc: In function 'rtx_code simplify_comparison(rtx_code,
rtx_def**, rtx_def**)':
    > .../gcc/combine.cc:12101:25: error: '*(unsigned int*)((char*)&inner_mode
+ offsetof(scalar_int_mode, scalar_int_mode::m_mode))' may be used
uninitialized [-Werror=maybe-uninitialized]
    > 12101 |   scalar_int_mode mode, inner_mode, tmode;
    >       |                         ^~~~~~~~~~

    I don't see how it could ever work properly, inner_mode in that spot is
    just uninitialized.

    I think we shouldn't worry about paradoxical subregs of non-scalar_int_mode
    REGs/MEMs and for the scalar_int_mode ones should initialize inner_mode
    before we use it.
    Another option would be to use
    maybe_lt (GET_MODE_PRECISION (GET_MODE (SUBREG_REG (op0))), BITS_PER_WORD)
    and
    load_extend_op (GET_MODE (SUBREG_REG (op0))) == ZERO_EXTEND,
    or set machine_mode smode = GET_MODE (SUBREG_REG (op0)); and use it in
    those two spots.

    2024-03-04  Jakub Jelinek  <jakub@redhat.com>

            PR rtl-optimization/113010
            * combine.cc (simplify_comparison): Guard the
            WORD_REGISTER_OPERATIONS check on scalar_int_mode of SUBREG_REG
            and initialize inner_mode.