[Bug target/100217] New: [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[Bug target/100217] New: [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

            Bug ID: 100217
           Summary: [11/12 Regression] ICE when building valgrind
                    testsuite with -march=z14 since r11-7552
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jakub at gcc dot gnu.org
  Target Milestone: ---

The following testcase reduced from valgrind ICEs on s390x with
-mlong-double-128 -march=z14 (with -O2 too):
void
foo (void)
{
  register long double f0 asm ("f0");
  f0 = 1.0L;
  asm("" : : "f" (f0));
}
starting with r11-7552-g3cb8aab390ccf31e4581863b080db30c6735e51e
fpext.i: In function ‘foo’:
fpext.i:6:3: internal compiler error: in gen_rtx_SUBREG, at emit-rtl.c:1023
    6 |   asm("" : : "f" (f0));
      |   ^~~
0xcbf9c2 gen_rtx_SUBREG(machine_mode, rtx_def*, poly_int<1u, unsigned long>)
        ../../gcc/emit-rtl.c:1023
0x1c0a54f gen_tf_to_fprx2(rtx_def*, rtx_def*)
        /usr/src/gcc-test/objz2/gcc/insn-emit.c:4851
0x1725bcb s390_md_asm_adjust
        ../../gcc/config/s390/s390.c:16786
0xb88cf4 expand_asm_stmt
        ../../gcc/cfgexpand.c:3424
0xb8a308 expand_gimple_stmt_1
        ../../gcc/cfgexpand.c:3841
0xb8a9af expand_gimple_stmt
        ../../gcc/cfgexpand.c:4008
0xb92fad expand_gimple_basic_block
        ../../gcc/cfgexpand.c:6045
0xb94da2 execute
        ../../gcc/cfgexpand.c:6729

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |11.0
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
                 CC|                            |iii at gcc dot gnu.org,
                   |                            |krebbel at gcc dot gnu.org
   Last reconfirmed|                            |2021-04-22

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

--- Comment #1 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Bet the new s390_md_asm_adjust code is unprepared to see hard registers there.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 50658
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50658&action=edit
gcc11-pr100217.patch

Untested fix.  IMHO when we have a hard reg in the inline asm, we just need to
honor it, trying to force it into a pseudo and then subreg would just mean the
user chosen reg is not guaranteed anymore.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[iii at linux dot ibm.com]

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

Ilya Leoshkevich <iii at linux dot ibm.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |iii at linux dot ibm.com

--- Comment #3 from Ilya Leoshkevich <iii at linux dot ibm.com> ---
There main problem here is that `register long double f0 asm ("f0")` does not
make sense on z14 anymore. long doubles are stored in vector registers now, not
in floating-point register pairs. If we skip the hard reg, the code will end up
having the following semantics:

vr0[0:128] = 1.0L;
asm("/* expect the value in vr0[0:64] . vr2[0:64] */");

and fail during the run time. So I think it's better to use the "best effort"
approach and force it into a pseudo, even if this would mean that the
user-specified register is not honored:

--- a/gcc/config/s390/s390.c
+++ b/gcc/config/s390/s390.c
@@ -16814,6 +16814,12 @@ s390_md_asm_adjust (vec<rtx> &outputs, vec<rtx>
&inputs,
       gcc_assert (allows_reg);
       /* Copy input value from a vector register into a FPR pair.  */
       rtx fprx2 = gen_reg_rtx (FPRX2mode);
+      if (REG_P (inputs[i]) && HARD_REGISTER_P (inputs[i]))
+       {
+         rtx orig_input = inputs[i];
+         inputs[i] = gen_reg_rtx (TFmode);
+         emit_move_insn (inputs[i], orig_input);
+       }
       emit_insn (gen_tf_to_fprx2 (fprx2, inputs[i]));
       inputs[i] = fprx2;
       input_modes[i] = FPRX2mode;

I need to check whether we can keep the output logic as is.

Ideally the code should be adapted and use the __LONG_DOUBLE_VX__ macro like
this:

#ifdef __LONG_DOUBLE_VX__
  register long double f0 asm ("v0");
#else
  register long double f0 asm ("f0");
#endif

  f0 = 1.0L;

#ifdef __LONG_DOUBLE_VX__
  asm("" : : "v" (f0));
#else
  asm("" : : "f" (f0));
#endif

Maybe a warning recommending to do this should be printed.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That seems like quite undesirable API change.
Can't the backend when it sees long double register vars for the fN registers
change the mode from TFmode to that new FPRX2mode, so that old code keeps
working?

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[iii at linux dot ibm.com]

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

--- Comment #5 from Ilya Leoshkevich <iii at linux dot ibm.com> ---
That would be an ideal solution, but I wonder how to implement it? Suppose we
find a way to convince expand to pick FPRX2mode for such a long double. What if
the following comes up?

register long double x asm ("v0");  /* FPRX2mode */
long double y;                      /* TFmode */
x += y;                             /* convert? */

Would it be feasible to also teach expand to do the mode conversions?



One other alternative might be to detect `register long double asm("fN")`
declarations and go back to using floating point register pairs for functions
that contain them.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

--- Comment #6 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Ilya Leoshkevich from comment #5)
> That would be an ideal solution, but I wonder how to implement it? Suppose
> we find a way to convince expand to pick FPRX2mode for such a long double.
> What if the following comes up?
> 
> register long double x asm ("v0");  /* FPRX2mode */
> long double y;                      /* TFmode */
> x += y;                             /* convert? */
> 
> Would it be feasible to also teach expand to do the mode conversions?

It is certainly doable, but perhaps with extra target hooks or something
similar.
Types have their TYPE_MODE and decls have DECL_MODE, though the question is
what breaks if TYPE_MODE != DECL_MODE, at least the comment in tree.h says that
they can only differ for FIELD_DECLs.  Anyway, in GIMPLE register vars are
non-SSA, so apart from inline asm one needs separate loads and stores to them,
so if we could expand those as having FPRX2 hard reg and loads from it convert
to TFmode and stores into it convert from TFmode, ...

> One other alternative might be to detect `register long double asm("fN")`
> declarations and go back to using floating point register pairs for
> functions that contain them.

But this might be actually best short-time solution (for GCC 11.x).

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
That said, I'm afraid I don't really understand what wrong happens with the
patch I've attached.
Trying something like:
long double
foo (void)
{
  register long double f0 asm ("f0");
  f0 = 1.0L;
  f0 += 127.L;
  f0 *= 32.L;
  return f0;
}
with -O0 -march=z14 -mlong-double-128 so that it is not all folded immediately
shows in the end the computations are done in vector registers.
And another thing to try is intermix that with inline asm expecting those in
"+f" so that intermediate results are pushed to the floating point register
pair.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[iii at linux dot ibm.com]

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

--- Comment #8 from Ilya Leoshkevich <iii at linux dot ibm.com> ---
Yeah, inline asm seems to be problematic:

/home/iii/gcc/build/gcc/xgcc -B/home/iii/gcc/build/gcc/
/home/iii/gcc/gcc/testsuite/gcc.target/s390/vector/long-double-asm-hardreg.c   
-fdiagnostics-plain-output   -O2 -march=z14 -mzarch -S -o
long-double-asm-hardreg.s

with the patch from comment 2 produces:

foo:
.LFB0:
        .cfi_startproc
        larl    %r5,.L4
        vl      %v0,.L5-.L4(%r5),3
#APP
# 10
"/home/iii/gcc/gcc/testsuite/gcc.target/s390/vector/long-double-asm-hardreg.c"
1
        # %v0
# 0 "" 2
#NO_APP
        br      %r14

`vl      %v0,.L5-.L4(%r5),3` loads 1.0L into %v0[0:128]. However, it should be
loaded into %v0[0:64] . %v2[0:64].

With the patch from comment 3 I get:

foo:
.LFB0:
        .cfi_startproc
        larl    %r5,.L4
        ld      %f0,.L5-.L4(%r5)
        ld      %f2,.L5-.L4+8(%r5)
#APP
# 10
"/home/iii/gcc/gcc/testsuite/gcc.target/s390/vector/long-double-asm-hardreg.c"
1
        # %f0
# 0 "" 2
#NO_APP
        br      %r14

which is correct, but in general case the exact reg that the user requested is
not honored.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[iii at linux dot ibm.com]

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

--- Comment #9 from Ilya Leoshkevich <iii at linux dot ibm.com> ---
Created attachment 50679
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50679&action=edit
regtesting this patch now

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|11.0                        |11.2

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
GCC 11.1 has been released, retargeting bugs to GCC 11.2.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[mark at gcc dot gnu.org]

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

--- Comment #11 from Mark Wielaard <mark at gcc dot gnu.org> ---
BTW. Disabling that test in valgrind produces another crash in another test
that looks similar:

gcc -DHAVE_CONFIG_H -I. -I../../..  -I../../.. -I../../../include
-I../../../coregrind -I../../../include -I../../../VEX/pub -I../../../VEX/pub
-DVGA_s390x=1 -DVGO_linux=1 -DVGP_s390x_linux=1 -DVGPV_s390x_linux_vanilla=1   
-Winline -Wall -Wshadow -Wno-long-long -g   -m64   -c -o pfpo.o pfpo.c

during RTL pass: expand
pfpo.c: In function 'main':
pfpo.c:35:3: internal compiler error: in gen_rtx_SUBREG, at emit-rtl.c:1023
   35 |   asm volatile(".short 0x010a\n\t"                                     
\
      |   ^~~
pfpo.c:146:15: note: in expansion of macro 'PFPO'
  146 |         d32 = PFPO(f128_in[j], long double, _Decimal32,
PFPO_F128_TO_D32,
      |               ^~~~

The whole PFPO define is:

#define PFPO(initial, src_type, dst_type, fn_code, round, ret_code, cc) \
({                                                                      \
  register src_type src_reg asm("f4") = initial;                        \
  register dst_type dst_reg asm("f0");                                  \
  register unsigned long fn asm("0") = fn_code | (round & 0xf);         \
  register unsigned int ret asm("1");                                   \
  asm volatile(".short 0x010a\n\t"                                      \
               "ipm %2\n\t"                                             \
               "srl %2,28\n\t"                                          \
               :"=f"(dst_reg), "=d"(ret), "=d" (cc)                     \
               : "f"(src_reg), "d"(fn));                                \
  ret_code = ret;                                                       \
  dst_reg;                                                              \
})

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
For valgrind, the quick workaround would be -march=z13 when compiling the s390x
tests that have register long double variables.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[mark at gcc dot gnu.org]

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

--- Comment #13 from Mark Wielaard <mark at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #12)
> For valgrind, the quick workaround would be -march=z13 when compiling the
> s390x tests that have register long double variables.

Yes, this works, if fpext and pfpo are build with -march=z13 they compile (and
the tests pass under valgrind).

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[cvs-commit at gcc dot gnu.org]

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

--- Comment #14 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Ilya Leoshkevich <iii@gcc.gnu.org>:

https://gcc.gnu.org/g:4f48c335d36674f90046b2823f0ac1c0545dc082

commit r12-379-g4f48c335d36674f90046b2823f0ac1c0545dc082
Author: Ilya Leoshkevich <iii@linux.ibm.com>
Date:   Mon Apr 26 14:12:08 2021 +0200

    IBM Z: Handle hard registers in s390_md_asm_adjust()

    gen_fprx2_to_tf() and gen_tf_to_fprx2() cannot handle hard registers,
    since the subregs they create do not pass validation.  Change
    s390_md_asm_adjust() to manually copy between hard VRs and FPRs instead
    of using these two functions.

    gcc/ChangeLog:

            PR target/100217
            * config/s390/s390.c (s390_hard_fp_reg_p): New function.
            (s390_md_asm_adjust): Handle hard registers.

    gcc/testsuite/ChangeLog:

            PR target/100217
            * gcc.target/s390/vector/long-double-asm-in-out-hard-fp-reg.c: New
test.
            * gcc.target/s390/vector/long-double-asm-inout-hard-fp-reg.c: New
test.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[cvs-commit at gcc dot gnu.org]

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

--- Comment #15 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-11 branch has been updated by Ilya Leoshkevich
<iii@gcc.gnu.org>:

https://gcc.gnu.org/g:445ce3cfb6832ec838caa10f2337c3bd00213517

commit r11-8360-g445ce3cfb6832ec838caa10f2337c3bd00213517
Author: Ilya Leoshkevich <iii@linux.ibm.com>
Date:   Mon Apr 26 14:12:08 2021 +0200

    IBM Z: Handle hard registers in s390_md_asm_adjust()

    gen_fprx2_to_tf() and gen_tf_to_fprx2() cannot handle hard registers,
    since the subregs they create do not pass validation.  Change
    s390_md_asm_adjust() to manually copy between hard VRs and FPRs instead
    of using these two functions.

    gcc/ChangeLog:

            PR target/100217
            * config/s390/s390.c (s390_hard_fp_reg_p): New function.
            (s390_md_asm_adjust): Handle hard registers.

    gcc/testsuite/ChangeLog:

            PR target/100217
            * gcc.target/s390/vector/long-double-asm-in-out-hard-fp-reg.c: New
test.
            * gcc.target/s390/vector/long-double-asm-inout-hard-fp-reg.c: New
test.

[Bug target/100217] [11/12 Regression] ICE when building valgrind testsuite with -march=z14 since r11-7552

[jakub at gcc dot gnu.org]

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

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

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

--- Comment #16 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Fixed.