[Bug target/97457] New: [10/11 Regression] SVE: wrong code since r10-4752-g2d56600c

[Bug target/97457] New: [10/11 Regression] SVE: wrong code since r10-4752-g2d56600c

[acoplan at gcc dot gnu.org]

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

            Bug ID: 97457
           Summary: [10/11 Regression] SVE: wrong code since
                    r10-4752-g2d56600c
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: acoplan at gcc dot gnu.org
  Target Milestone: ---

AArch64 GCC miscompiles the following testcase:

int a;
long c;
signed char d(char e, char f) { return e + f; }
int main(void) {
  for (; a <= 1; a++) {
    c = -8;
    for (; c != 3; c = d(c, 1))
      ;
  }
  char b = c;
  if (b != 3)
    __builtin_abort();
}

with -O3 -march=armv8.2-a+sve since
r10-4752-g2d56600c8de397d09a16dedd33d310a763a832ae:

commit 2d56600c8de397d09a16dedd33d310a763a832ae
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Sat Nov 16 11:14:51 2019

    [AArch64] Add truncation for partial SVE modes

On a machine with 512-bit SVE vectors, we end up with c = 11 and hit the call
to abort.

The generated code at r10-4752 is as follows. I've annotated it with the
runtime behaviour of a machine with 512-bit SVE vectors.

main:
        addvl   sp, sp, #-1
        adrp    x7, a
        sub     sp, sp, #16
        stp     x29, x30, [sp]
        mov     x29, sp
        ldr     w4, [x7, #:lo12:a]
        cmp     w4, 1
        bgt     .L20
        cntd    x6          // x6 := 8
        neg     w3, w6      // w3 := -8
        add     w3, w3, 11  // w3 := 3
        sub     w6, w6, #1  // x6 := 7
        ptrue   p0.b, all
        pfalse  p1.b
        .p2align 3,,7
.L4:
        cmp     w6, 10
        bhi     .L10        // not taken
        mov     w0, 0
        index   z1.d, #-8, #1
        .p2align 3,,7
.L7:
        incd    x0          // x0 := 8
        mov     z0.d, z1.d
        cmp     w0, w3
        add     z0.b, z0.b, #1
        incd    z1.d
        sxtb    z2.d, p0/m, z0.d
        bls     .L7         // not taken
        add     x1, sp, 16
        addvl   x2, sp, #1
        st1b    z0.d, p0, [x1, #7, mul vl]
        uxtw    x1, w0      // x1 := 8
        cmp     w0, 11
        ldrb    w5, [x2, 15]
        sub     x2, x1, #8  // x2 := 0
        lastb   x1, p1, z2.d
        beq     .L8         // not taken
        add     w1, w2, 1   // w1 := 1
        cmp     w0, 10
        and     w5, w1, 255 // w5 := 1
        sxtb    x1, w1      // x1 := 1
        beq     .L8
.L6:
        add     w1, w5, 10  // w1 := 11
        and     w5, w1, 255 // w5 := 11
        sxtb    x1, w1      // x1 := 11
.L8:
        add     w4, w4, 1   // outer loop induction variable (a)
        cmp     w4, 2
        bne     .L4         // taken 1st time round, x0 reset to 0 at top of
loop
        adrp    x0, c
        str     w4, [x7, #:lo12:a] // write a back (a = 2)
        str     x1, [x0, #:lo12:c] // write c back (c = 11!)
.L5:
        cmp     w5, 3
        bne     .L22        // boom! (w5 = 11)
        ldp     x29, x30, [sp]
        addvl   sp, sp, #1
        mov     w0, 0
        add     sp, sp, 16
        ret
        .p2align 2,,3
.L10:
        mov     w5, 249
        b       .L6
.L20:
        adrp    x0, c
        ldrb    w5, [x0, #:lo12:c]
        b       .L5
.L22:
        bl      abort
        .size   main, .-main
        .comm   c,8,8
        .comm   a,4,4
        .ident  "GCC: (unknown) 10.0.0 20191116 (experimental)"

[Bug target/97457] [10/11 Regression] SVE: wrong code since r10-4752-g2d56600c

[acoplan at gcc dot gnu.org]

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

Alex Coplan <acoplan at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
      Known to fail|                            |11.0
           Keywords|                            |wrong-code
             Target|                            |aarch64
   Target Milestone|---                         |10.3
                 CC|                            |richard.sandiford at arm dot com

[Bug target/97457] [10/11 Regression] SVE: wrong code since r10-4752-g2d56600c

[acoplan at gcc dot gnu.org]

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

--- Comment #1 from Alex Coplan <acoplan at gcc dot gnu.org> ---
To be clear, the second beq .L8 is in the body of the main loop is not taken
either in the execution described here. The lack of a comment there might have
suggested otherwise.

[Bug target/97457] [10/11 Regression] SVE: wrong code since r10-4752-g2d56600c

[acoplan at gcc dot gnu.org]

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

--- Comment #2 from Alex Coplan <acoplan at gcc dot gnu.org> ---
For the similar testcase:

long a;
short b;
signed char c(char d, char e) { return d + e; }
int main(void) {
  a = -30;
  for (; a < 24; a = c(a, 5)) {
    short *f = &b;
    (*f)--;
  }
  if (b != -11)
    __builtin_abort();
}

we fail to assemble it after r10-4752. This is fixed by
r10-5304-g30f8bf3d6c072a8fce14e8a003dff485a9068a97, but we have wrong code
thereafter.

[Bug target/97457] [10/11 Regression] SVE: wrong code since r10-4752-g2d56600c

[rsandifo at gcc dot gnu.org]

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

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
           Assignee|unassigned at gcc dot gnu.org      |rsandifo at gcc dot gnu.org
                 CC|                            |rsandifo at gcc dot gnu.org
             Status|UNCONFIRMED                 |ASSIGNED
   Last reconfirmed|                            |2020-10-27

--- Comment #3 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Ugh.  This is yet another problem with calculating value ranges
for POLY_INT_CSTs.  We have:

  ivtmp_76 = ASSERT_EXPR <ivtmp_60, ivtmp_60 > POLY_INT_CST [9, 4294967294]>

where the VQ coefficient is unsigned but is effectively acting
as a negative number.  We wrongly give the POLY_INT_CST the range:

  [9, INT_MAX]

and things go downhill from there.

I guess this is the final nail in the coffin for doing VRP on
POLY_INT_CSTs. :-(  For other similarly exotic testcases we could
have overflow for any coefficient, not just those that could
be treated as contextually negative.

Let's see what the fallout is from removing the code...

[Bug target/97457] [10/11 Regression] SVE: wrong code since r10-4752-g2d56600c

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:54ef7701a9dec8c923a12d1983f8a051ba88a7b9

commit r11-4495-g54ef7701a9dec8c923a12d1983f8a051ba88a7b9
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Wed Oct 28 19:05:49 2020 +0000

    value-range: Give up on POLY_INT_CST ranges [PR97457]

    This PR shows another problem with calculating value ranges for
    POLY_INT_CSTs.  We have:

      ivtmp_76 = ASSERT_EXPR <ivtmp_60, ivtmp_60 > POLY_INT_CST [9,
4294967294]>

    where the VQ coefficient is unsigned but is effectively acting
    as a negative number.  We wrongly give the POLY_INT_CST the range:

      [9, INT_MAX]

    and things go downhill from there: later iterations of the unrolled
    epilogue are wrongly removed as dead.

    I guess this is the final nail in the coffin for doing VRP on
    POLY_INT_CSTs.  For other similarly exotic testcases we could have
    overflow for any coefficient, not just those that could be treated
    as contextually negative.

    Testing TYPE_OVERFLOW_UNDEFINED doesn't seem like an option because we
    couldn't handle warn_strict_overflow properly.  At this stage we're
    just recording a range that might or might not lead to strict-overflow
    assumptions later.

    It still feels like we should be able to do something here, but for
    now removing the code seems safest.  It's also telling that there
    are no testsuite failures on SVE from doing this.

    gcc/
            PR tree-optimization/97457
            * value-range.cc (irange::set): Don't decay POLY_INT_CST ranges
            to integer ranges.

    gcc/testsuite/
            PR tree-optimization/97457
            * gcc.dg/vect/pr97457.c: New test.

[Bug target/97457] [10 Regression] SVE: wrong code since r10-4752-g2d56600c

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Richard Sandiford
<rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:75a5af680a1788ba844902fd0681958da8e2a4ce

commit r10-9112-g75a5af680a1788ba844902fd0681958da8e2a4ce
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Wed Dec 2 18:39:24 2020 +0000

    value-range: Give up on POLY_INT_CST ranges [PR97457]

    This PR shows another problem with calculating value ranges for
    POLY_INT_CSTs.  We have:

      ivtmp_76 = ASSERT_EXPR <ivtmp_60, ivtmp_60 > POLY_INT_CST [9,
4294967294]>

    where the VQ coefficient is unsigned but is effectively acting
    as a negative number.  We wrongly give the POLY_INT_CST the range:

      [9, INT_MAX]

    and things go downhill from there: later iterations of the unrolled
    epilogue are wrongly removed as dead.

    I guess this is the final nail in the coffin for doing VRP on
    POLY_INT_CSTs.  For other similarly exotic testcases we could have
    overflow for any coefficient, not just those that could be treated
    as contextually negative.

    Testing TYPE_OVERFLOW_UNDEFINED doesn't seem like an option because we
    couldn't handle warn_strict_overflow properly.  At this stage we're
    just recording a range that might or might not lead to strict-overflow
    assumptions later.

    It still feels like we should be able to do something here, but for
    now removing the code seems safest.  It's also telling that there
    are no testsuite failures on SVE from doing this.

    gcc/
            PR tree-optimization/97457
            * value-range.cc (irange::set): Don't decay POLY_INT_CST ranges
            to integer ranges.

    gcc/testsuite/
            PR tree-optimization/97457
            * gcc.dg/vect/pr97457.c: New test.

    (cherry picked from commit 54ef7701a9dec8c923a12d1983f8a051ba88a7b9)

[Bug target/97457] [10 Regression] SVE: wrong code since r10-4752-g2d56600c

[rsandifo at gcc dot gnu.org]

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

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

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

--- Comment #6 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Fixed on trunk and GCC 10 branch.

[Bug target/97457] [10 Regression] SVE: wrong code since r10-4752-g2d56600c

[rsandifo at gcc dot gnu.org]

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

--- Comment #7 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
*** Bug 97400 has been marked as a duplicate of this bug. ***