[Bug target/111867] New: aarch64: Wrong code for bf16 literal load when the arch support +fp16

[Bug target/111867] New: aarch64: Wrong code for bf16 literal load when the arch support +fp16

[iains at gcc dot gnu.org]

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

            Bug ID: 111867
           Summary: aarch64: Wrong code for bf16 literal load when the
                    arch support +fp16
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: iains at gcc dot gnu.org
  Target Milestone: ---

Analysing some target fails for aarch64-darwin.

The base arch for Apple M1 is (as far as I can determine)
armv8.4-a+fp16+sb+ssbs
So it has fp16 and fp15fml 0 but not bf16.

(M2 does have bf16).

===

int main ()
{
  __bf16 a = 1.0bf16;
  return (int) (a + a);
}

=== with the arch flags above produces:
_main:
 <snip>
        fmov    h31, 1.0e+0
        str     h31, [x29, 46]
        ldr     h15, [x29, 46]
        mov     v0.h[0], v15.h[0]
        bl      ___extendbfsf2
        fmov    s14, s0
<snip>

Which seems to be loading an __fp16 value into h31 (not a __bf16 value)
not surprisingly this fails.


I checked the instruction bit pattern with objdump, and it is 0x1eee101f, which
is clearly a fp16 load.


==== with pruned arch flags armv8.4-a

_main:
 <snip>
        adrp    x0, lC0@PAGE
        ldr     h31, [x0, #lC0@PAGEOFF]
        str     h31, [x29, 30]
        ldr     h0, [x29, 30]
        bl      ___extendbfsf2
<snip>


lC0:
        .hword  16256

which looks correct (and produces the expected answer).
----
So support for fp16 seems to be breaking soft __bf16.

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
             Target|aarch64-linux-gnu,          |aarch64
                   |aarch64-apple-darwin        |
   Last reconfirmed|                            |2023-10-18
     Ever confirmed|0                           |1

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
This is definitely wrong:
(insn 5 2 6 (set (reg:BF 63 v31 [95])
        (const_double:BF 1.0e+0 [0x0.8p+1])) "/app/example.cpp":4:10 72
{*movbf_aarch64}
     (nil))


Confirmed.

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Maybe something like:
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 62b1ae0652f..db2dde84329 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -23788,7 +23788,8 @@ aarch64_float_const_representable_p (rtx x)
     return false;

   if (GET_MODE (x) == VOIDmode
-      || (GET_MODE (x) == HFmode && !TARGET_FP_F16INST))
+      || (GET_MODE (x) == HFmode && !TARGET_FP_F16INST)
+      || (GET_MODE (x) == BFmode))
     return false;

   r = *CONST_DOUBLE_REAL_VALUE (x);

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #2)
> Maybe something like:
> diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> index 62b1ae0652f..db2dde84329 100644
> --- a/gcc/config/aarch64/aarch64.cc
> +++ b/gcc/config/aarch64/aarch64.cc
> @@ -23788,7 +23788,8 @@ aarch64_float_const_representable_p (rtx x)
>      return false;
> 
>    if (GET_MODE (x) == VOIDmode
> -      || (GET_MODE (x) == HFmode && !TARGET_FP_F16INST))
> +      || (GET_MODE (x) == HFmode && !TARGET_FP_F16INST)
> +      || (GET_MODE (x) == BFmode))
>      return false;
> 
>    r = *CONST_DOUBLE_REAL_VALUE (x);

That is there are no fmov instructions for bfmode constants ...

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[iains at gcc dot gnu.org]

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

--- Comment #4 from Iain Sandoe <iains at gcc dot gnu.org> ---
(In reply to Andrew Pinski from comment #3)
> (In reply to Andrew Pinski from comment #2)
> > Maybe something like:
> > diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
> > index 62b1ae0652f..db2dde84329 100644
> > --- a/gcc/config/aarch64/aarch64.cc
> > +++ b/gcc/config/aarch64/aarch64.cc
> > @@ -23788,7 +23788,8 @@ aarch64_float_const_representable_p (rtx x)
> >      return false;
> > 
> >    if (GET_MODE (x) == VOIDmode
> > -      || (GET_MODE (x) == HFmode && !TARGET_FP_F16INST))
> > +      || (GET_MODE (x) == HFmode && !TARGET_FP_F16INST)
> > +      || (GET_MODE (x) == BFmode))
> >      return false;
> > 
> >    r = *CONST_DOUBLE_REAL_VALUE (x);

Yeah that fixes this case; re-running the testsuite to see if that clears any
other bf16 fails.


> That is there are no fmov instructions for bfmode constants ...

Although I notice there's a spare bit pattern in the "ftype" field [0b10) in
the fmov insn... but I guess that's being kept for something more useful than
bf16.

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |pinskia at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #5 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I will handle this next week (after I start my new job).

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |NEW
           Assignee|pinskia at gcc dot gnu.org         |unassigned at gcc dot gnu.org

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Won't be able to get to this until assignment is figured out ....

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |testsuite-fail
           Assignee|unassigned at gcc dot gnu.org      |pinskia at gcc dot gnu.org
             Status|NEW                         |ASSIGNED

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
FAIL: gcc.dg/torture/bfloat16-basic.c   -O0  execution test
FAIL: gcc.dg/torture/bfloat16-basic.c   -O1  execution test
FAIL: gcc.dg/torture/bfloat16-basic.c   -O2  execution test
FAIL: gcc.dg/torture/bfloat16-basic.c   -O3 -g  execution test
FAIL: gcc.dg/torture/bfloat16-basic.c   -Os  execution test
FAIL: gcc.dg/torture/bfloat16-basic.c   -O2 -flto -fno-use-linker-plugin
-flto-partition=none  execution test
FAIL: gcc.dg/torture/bfloat16-basic.c   -O2 -flto -fuse-linker-plugin
-fno-fat-lto-objects  execution test
FAIL: gcc.dg/torture/bfloat16-builtin.c   -O0  execution test
FAIL: gcc.dg/torture/bfloat16-builtin.c   -O1  execution test
FAIL: gcc.dg/torture/bfloat16-builtin.c   -O2  execution test
FAIL: gcc.dg/torture/bfloat16-builtin.c   -O3 -g  execution test
FAIL: gcc.dg/torture/bfloat16-builtin.c   -Os  execution test
FAIL: gcc.dg/torture/bfloat16-builtin.c   -O2 -flto -fno-use-linker-plugin
-flto-partition=none  execution test
FAIL: gcc.dg/torture/bfloat16-builtin.c   -O2 -flto -fuse-linker-plugin
-fno-fat-lto-objects  execution test


Fail when tested with `-march=armv9-a+sve2`.

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

--- Comment #8 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Created attachment 56842
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=56842&action=edit
Patch which I will be submitting soon

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Patch submitted:
https://gcc.gnu.org/pipermail/gcc-patches/2023-December/640028.html

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The trunk branch has been updated by Andrew Pinski <pinskia@gcc.gnu.org>:

https://gcc.gnu.org/g:35ade856eaafd9c39ce864b25d127e1f98a3bb57

commit r14-6421-g35ade856eaafd9c39ce864b25d127e1f98a3bb57
Author: Andrew Pinski <quic_apinski@quicinc.com>
Date:   Sat Dec 9 20:02:24 2023 -0800

    aarch64: Fix wrong code for bfloat when f16 is enabled [PR 111867]

    The problem here is when f16 is enabled, movbf_aarch64 accepts `Ufc`
    as a constraint:
         [ w        , Ufc ; fconsts     , fp16  ] fmov\t%h0, %1
    But that is for fmov values and in this case fmov represents f16 rather
than bfloat16 values.
    This means we would get the wrong value in the register.

    Built and tested for aarch64-linux-gnu with no regressions.  Also tested
with `-march=armv9-a+sve2,
    gcc.dg/torture/bfloat16-basic.c and gcc.dg/torture/bfloat16-builtin.c no
longer fail.

    gcc/ChangeLog:

            PR target/111867
            * config/aarch64/aarch64.cc (aarch64_float_const_representable_p):
For BFmode,
            only accept +0.0.

    Signed-off-by: Andrew Pinski <quic_apinski@quicinc.com>

[Bug target/111867] aarch64: Wrong code for bf16 literal load when the arch support +fp16

[pinskia at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
   Target Milestone|---                         |14.0
         Resolution|---                         |FIXED

--- Comment #11 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Fixed.