[Bug target/95526] New: aarch64: Wrong code accessing complex number from varargs

[Bug target/95526] New: aarch64: Wrong code accessing complex number from varargs

[acoplan at gcc dot gnu.org]

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

            Bug ID: 95526
           Summary: aarch64: Wrong code accessing complex number from
                    varargs
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: major
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: acoplan at gcc dot gnu.org
  Target Milestone: ---

Commit eb72dc663e9070b281be83a80f6f838a3a878822 introduces a wrong code bug on
AArch64.

This causes the test case gcc/testsuite/gcc.dg/complex-1.c to start failing.
This test case can be reduced to the following program:

  extern void abort(void);

  void f(unsigned x, ...)
  {
    __builtin_va_list ap;
    _Complex float cf;

    __builtin_va_start(ap, x);
    cf = __builtin_va_arg(ap, _Complex float);
    __builtin_va_end(ap);

    if (__imag__ cf != 2.0f)
      abort();
  }

  int main(void)
  {
    f(0, 2.0fi);
  }

which calls abort() at both -O0 and -O2 after this patch. Prior to this patch,
we get the following code for f at -O2:

f:
.LFB0:
        .cfi_startproc
        stp     x29, x30, [sp, -80]!
        .cfi_def_cfa_offset 80
        .cfi_offset 29, -80
        .cfi_offset 30, -72
        mov     x29, sp
        str     q1, [sp, 64]
        fmov    s1, 2.0e+0
        add     x0, sp, 80
        ldr     s2, [sp, 64]
        stp     x0, x0, [sp, 16]
        fcmp    s2, s1
        str     x0, [sp, 32]
        stp     wzr, wzr, [sp, 40]
        str     q0, [sp, 48]
        bne     .L8
        ldp     x29, x30, [sp], 80
        .cfi_remember_state
        .cfi_restore 30
        .cfi_restore 29
        .cfi_def_cfa_offset 0
        ret
.L8:
        .cfi_restore_state
        bl      abort
        .cfi_endproc

and now, we get:

f:
.LFB0:
        .cfi_startproc
        stp     x29, x30, [sp, -80]!
        .cfi_def_cfa_offset 80
        .cfi_offset 29, -80
        .cfi_offset 30, -72
        mov     w0, -32
        add     x1, sp, 80
        mov     x29, sp
        stp     x1, x1, [sp, 16]
        str     x1, [sp, 32]
        stp     wzr, w0, [sp, 40]
        str     q0, [sp, 48]
        str     q1, [sp, 64]
        bl      abort
        .cfi_endproc

which appears to unconditionally call abort().

[Bug target/95526] [11 Regression] aarch64: Wrong code accessing complex number from varargs

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org
     Ever confirmed|0                           |1
            Summary|aarch64: Wrong code         |[11 Regression] aarch64:
                   |accessing complex number    |Wrong code accessing
                   |from varargs                |complex number from varargs
           Keywords|                            |wrong-code
   Last reconfirmed|                            |2020-06-04
   Target Milestone|---                         |11.0
             Status|UNCONFIRMED                 |ASSIGNED

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Taking a look.  I suspect an issue with VA_ARG gimplification.

[Bug target/95526] [11 Regression] aarch64: Wrong code accessing complex number from varargs

[rguenth at gcc dot gnu.org]

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

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
                 CC|                            |rearnsha at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org
           Keywords|                            |patch
           Priority|P3                          |P1
           Assignee|rguenth at gcc dot gnu.org         |unassigned at gcc dot gnu.org

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
Indeed.  Diff from gcc 10 on the stdarg output is:

-  REALPART_EXPR <ha.3> = _21;
-  _23 = ap.__vr_top;
-  _24 = (sizetype) _13;
-  _25 = _24 + 16;
-  _26 = _23 + _25;
-  _27 = MEM[(float *)_26];
-  MEM[(float *)&ha.3 + 4B] = _27;
-  iftmp.5_29 = &ha.3;
+  _23 = IMAGPART_EXPR <ha.3_22(D)>;
+  ha.3_24 = COMPLEX_EXPR <_21, _23>;
+  _25 = ap.__vr_top;
+  _26 = (sizetype) _13;
+  _27 = _26 + 16;
+  _28 = _25 + _27;
+  _29 = MEM[(float *)_28];
+  MEM[(float *)&ha.6 + 4B] = _29;
+  ha.6 = ha.3_24;
+  iftmp.5_32 = &ha.6;

note how the trunk version overwrites ha.6 with ha.3_24 which contains only
the REALPART while the imagpart write is lost.

And the following fixes the latent bug - we failed to mark 'ha.6'
TREE_ADDRESSABLE (address-taken):

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index 6352d4ff78a..97da6076239 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -16370,6 +16370,7 @@ aarch64_gimplify_va_arg_expr (tree valist, tree type,
gimple_seq *pre_p,
        }

       /* *(field_ptr_t)&ha = *((field_ptr_t)vr_saved_area  */
+      TREE_ADDRESSABLE (tmp_ha) = 1;
       tmp_ha = build1 (ADDR_EXPR, field_ptr_t, tmp_ha);
       addr = t;
       t = fold_convert (field_ptr_t, addr);


I'm not set up for aarch64 bootstrap / retest.  Richard, can you take
the honors here?  Thanks.

[Bug target/95526] [11 Regression] aarch64: Wrong code accessing complex number from varargs

[acoplan at gcc dot gnu.org]

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

--- Comment #3 from Alex Coplan <acoplan at gcc dot gnu.org> ---
I'm happy to test the fix.

[Bug target/95526] [11 Regression] aarch64: Wrong code accessing complex number from varargs

[acoplan at gcc dot gnu.org]

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

--- Comment #4 from Alex Coplan <acoplan at gcc dot gnu.org> ---
Ok, bootstrap and test looks good. Cleaned up a bunch of recent testsuite
failures:

+PASS: gcc.dg/compat/scalar-by-value-3 c_compat_x_tst.o-c_compat_y_tst.o
execute
+PASS: gcc.dg/compat/scalar-by-value-4 c_compat_x_tst.o-c_compat_y_tst.o
execute
+PASS: gcc.dg/compat/scalar-by-value-5 c_compat_x_tst.o-c_compat_y_tst.o
execute
+PASS: gcc.dg/compat/scalar-by-value-6 c_compat_x_tst.o-c_compat_y_tst.o
execute
+PASS: gcc.dg/compat/scalar-return-3 c_compat_x_tst.o-c_compat_y_tst.o execute
+PASS: gcc.dg/compat/scalar-return-4 c_compat_x_tst.o-c_compat_y_tst.o execute
+PASS: gcc.dg/complex-1.c execution test
+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -O0
+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -O1
+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -O2
+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -O3 -g
+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -Og -g
+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -Os

OK to commit?

[Bug target/95526] [11 Regression] aarch64: Wrong code accessing complex number from varargs

[rguenther at suse dot de]

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

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
On June 4, 2020 4:42:53 PM GMT+02:00, "acoplan at gcc dot gnu.org"
<gcc-bugzilla@gcc.gnu.org> wrote:
>https://gcc.gnu.org/bugzilla/show_bug.cgi?id=95526
>
>--- Comment #4 from Alex Coplan <acoplan at gcc dot gnu.org> ---
>Ok, bootstrap and test looks good. Cleaned up a bunch of recent
>testsuite
>failures:
>
>+PASS: gcc.dg/compat/scalar-by-value-3
>c_compat_x_tst.o-c_compat_y_tst.o
>execute
>+PASS: gcc.dg/compat/scalar-by-value-4
>c_compat_x_tst.o-c_compat_y_tst.o
>execute
>+PASS: gcc.dg/compat/scalar-by-value-5
>c_compat_x_tst.o-c_compat_y_tst.o
>execute
>+PASS: gcc.dg/compat/scalar-by-value-6
>c_compat_x_tst.o-c_compat_y_tst.o
>execute
>+PASS: gcc.dg/compat/scalar-return-3 c_compat_x_tst.o-c_compat_y_tst.o
>execute
>+PASS: gcc.dg/compat/scalar-return-4 c_compat_x_tst.o-c_compat_y_tst.o
>execute
>+PASS: gcc.dg/complex-1.c execution test
>+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -O0
>+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -O1
>+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -O2
>+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -O3 -g
>+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -Og -g
>+PASS: gcc.target/aarch64/aapcs64/va_arg-7.c execution,  -Os
>
>OK to commit?

Yes. The patch is quite obvious.

[Bug target/95526] [11 Regression] aarch64: Wrong code accessing complex number from varargs

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Alex Coplan <acoplan@gcc.gnu.org>:

https://gcc.gnu.org/g:ab56390384cd5168b548ff07e6f0c9c4d41420fb

commit r11-936-gab56390384cd5168b548ff07e6f0c9c4d41420fb
Author: Richard Biener <rguenther@suse.de>
Date:   Thu Jun 4 16:26:36 2020 +0100

    aarch64: PR target/95526: Fix gimplification of varargs

    This patch fixes a latent bug exposed by
    eb72dc663e9070b281be83a80f6f838a3a878822 in the aarch64 backend that was
    causing wrong codegen and several testsuite failures. See the discussion
    on the bug for details.

    Bootstrapped and regtested on aarch64-linux-gnu. Cleaned up several
    failing tests and no new fails introduced.

    2020-06-04  Richard Biener  <rguenther@suse.de>

    gcc/:

            * config/aarch64/aarch64.c (aarch64_gimplify_va_arg_expr):
            Ensure that tmp_ha is marked TREE_ADDRESSABLE.

[Bug target/95526] [11 Regression] aarch64: Wrong code accessing complex number from varargs

[acoplan at gcc dot gnu.org]

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

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

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

--- Comment #7 from Alex Coplan <acoplan at gcc dot gnu.org> ---
Thanks Richard. Fix committed on master.

[Bug target/95526] [11 Regression] aarch64: Wrong code accessing complex number from varargs

[acoplan at gcc dot gnu.org]

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

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

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

--- Comment #8 from Alex Coplan <acoplan at gcc dot gnu.org> ---
*** Bug 95055 has been marked as a duplicate of this bug. ***