Bug 107515 - MVE: Generic functions do not accept _Float16 scalars
Summary: MVE: Generic functions do not accept _Float16 scalars
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 12.2.0
: P3 normal
Target Milestone: ---
Assignee: Stam Markianos-Wright
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-11-03 09:27 UTC by Kevin Bracey
Modified: 2023-05-18 11:06 UTC (History)
2 users (show)

See Also:
Host:
Target: arm
Build:
Known to work:
Known to fail: 11.3.1
Last reconfirmed: 2022-11-10 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Bracey 2022-11-03 09:27:56 UTC
Compiling C code, generic functions taking floating point scalars in arm_mve.h do not accept `_Float16` values.

// Using gcc -mcpu=cortex-m55 -O2
// Uploaded at https://godbolt.org/z/7jrqWWroY

#include <arm_mve.h>

void test(void)
{
    float16x8_t x;

    x = vmulq(x, 0.5); // ok
    x = vmulq(x, 0.5f); // ok
    x = vmulq(x, (__fp16) 0.5); // ok
    x = vmulq(x, 0.15f16); // rejected
    x = vmulq(x, (_Float16) 0.15); // rejected
}

Output:

<source>:10:9: error: '_Generic' selector of type 'int (*)[4][39]' is not compatible with any association
   10 |     x = vmulq(x, 0.15f16); // rejected
      |         ^~~~~
<source>:11:9: error: '_Generic' selector of type 'int (*)[4][39]' is not compatible with any association
   11 |     x = vmulq(x, (_Float16) 0.15); // rejected
      |         ^~~~~
Comment 1 Stam Markianos-Wright 2022-11-10 17:30:18 UTC
Also confirmed on trunk and assigning to myself, because I believe I've found a fix: 
```
diff --git a/gcc/config/arm/arm_mve.h b/gcc/config/arm/arm_mve.h
index 2d213e12304..ce20a6fcd24 100644
--- a/gcc/config/arm/arm_mve.h
+++ b/gcc/config/arm/arm_mve.h
@@ -35582,6 +35582,9 @@ enum {
        short: __ARM_mve_type_int_n, \
        int: __ARM_mve_type_int_n, \
        long: __ARM_mve_type_int_n, \
+       _Float16: __ARM_mve_type_fp_n, \
+       __fp16: __ARM_mve_type_fp_n, \
+       float: __ARM_mve_type_fp_n, \
        double: __ARM_mve_type_fp_n, \
        long long: __ARM_mve_type_int_n, \
        unsigned char: __ARM_mve_type_int_n, \
```

Still untested, though, and I'll likely send it to the mailing list within the next week or two (we've got a couple more arm_mve.h changes in the pipeline, so I'll test them all together).
Comment 2 Kevin Bracey 2022-11-16 16:35:38 UTC
I've just spotted another apparent generic selection problem in my reproducer for  bug 107714 - should I create a new issue for it?
Comment 3 Stam Markianos-Wright 2022-11-17 16:56:12 UTC
(In reply to Kevin Bracey from comment #2)
> I've just spotted another apparent generic selection problem in my
> reproducer for  bug 107714 - should I create a new issue for it?

Our patch series has gone up to the mailing list! For _Float16 see:

https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606587.html

For `vmuq` this:

https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606575.html

works on my side, so if it gets merged and works for your case, too, then I guess we don't have to worry about a new bug report :)
Comment 4 Kevin Bracey 2022-11-21 12:24:40 UTC
Yes, looking at them it seems clear those patches address what I'm seeing with the `vmulq(x, 6)` issue.
Comment 5 GCC Commits 2022-11-28 09:12:06 UTC
The trunk branch has been updated by Andrea Corallo <akrl@gcc.gnu.org>:

https://gcc.gnu.org/g:2fefb8931d566cc8a4cbba81601972b0d2002f95

commit r13-4332-g2fefb8931d566cc8a4cbba81601972b0d2002f95
Author: Stam Markianos-Wright <stam.markianos-wright@arm.com>
Date:   Thu Nov 10 15:06:47 2022 +0000

    arm: Explicitly specify other float types for _Generic overloading [PR107515]
    
    This patch adds explicit references to other float types
    to __ARM_mve_typeid in arm_mve.h.  Resolves PR 107515:
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107515
    
    gcc/ChangeLog:
            PR target/107515
            * config/arm/arm_mve.h (__ARM_mve_typeid): Add float types.
Comment 6 Kevin Bracey 2022-11-29 08:59:22 UTC
Retesting the Godbolt on trunk, it's now worse - every line produces multiple not-very-informative errors:

source>:7:9: error: '_Generic' specifies two compatible types
    7 |     x = vmulq(x, 0.5); // ok
      |         ^~~~~
<source>:7:9: note: compatible type is here
    7 |     x = vmulq(x, 0.5); // ok
      |         ^~~~~

(repeated 6 times per source line)
Comment 7 Stam Markianos-Wright 2022-11-29 13:23:53 UTC
(In reply to Kevin Bracey from comment #6)
> Retesting the Godbolt on trunk, it's now worse - every line produces
> multiple not-very-informative errors:
> 
> source>:7:9: error: '_Generic' specifies two compatible types
>     7 |     x = vmulq(x, 0.5); // ok
>       |         ^~~~~
> <source>:7:9: note: compatible type is here
>     7 |     x = vmulq(x, 0.5); // ok
>       |         ^~~~~
> 
> (repeated 6 times per source line)

Interesting... Thanks for spotting this, I didn't see this in my testing, because it doesn't seem to happen on baremetal `arm-none-eabi` (and I still can't replicate it there), but I do see this on the linux target (let me know if you are seeing anything different). I am investigating further!
Comment 8 Kevin Bracey 2022-11-29 13:29:08 UTC
I'm only testing on the Linux trunk because it's what Godbolt has. If it has bare-metal, I'm not seeing it.

Actual real development system is bare-metal using Arm's embedded GCC releases, and I don't have a set-up to test a trunk GCC build on it at the moment.

Clearly Helium+Linux on Godbolt is a bit confused because it's always using non-existent registers Q8 upwards. There may be a fundamental config error leading to all sorts of strange results.

(Mostly reproduces my bare-metal findings though.)
Comment 9 Stam Markianos-Wright 2022-12-01 18:37:54 UTC
> Clearly Helium+Linux on Godbolt is a bit confused 

Yea, I agree -- it still shouldn't be an unintuitive front-end type clash error, though!

I've posted another patch: https://gcc.gnu.org/pipermail/gcc-patches/2022-December/607675.html (see there for what the error was --- interestingly it was coming from `__ARM_mve_coerce3` and it didn't directly have anything to do with the float types)
Comment 10 GCC Commits 2023-01-16 12:52:21 UTC
The master branch has been updated by Stam Markianos-Wright <stammark@gcc.gnu.org>:

https://gcc.gnu.org/g:8a1360e72d6c6056606aa5edd8c906c50f26de59

commit r13-5200-g8a1360e72d6c6056606aa5edd8c906c50f26de59
Author: Stam Markianos-Wright <stam.markianos-wright@arm.com>
Date:   Mon Jan 16 11:40:40 2023 +0000

    arm: Split up MVE _Generic associations to prevent type clashes [PR107515]
    
    With these previous patches:
    https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606586.html
    https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606587.html
    we enabled the MVE overloaded _Generic associations to handle more
    scalar types, however at PR 107515 we found a new regression that
    wasn't detected in our testing:
    
    With glibc's posix/types.h:
    ```
    typedef signed int __int32_t;
    ...
    typedef __int32_t int32_t;
    ```
    We would get a `error: '_Generic' specifies two compatible types`
    from `__ARM_mve_coerce3` because of `type: param`, when `type` is
    `int` and `int32_t: param` both being the same under the hood.
    
    The same did not happen with Newlib's header sys/_stdint.h:
    ```
    typedef long int __int32_t;
    ...
    typedef __int32_t int32_t ;
    ```
    which worked fine, because it uses `long int`.
    
    The same could feasibly happen in `__ARM_mve_coerce2` between
    `__fp16` and `float16_t`.
    
    The solution here is to break the _Generic down so that the similar
    types don't appear at the same level, as is done in `__ARM_mve_typeid`
    
    gcc/ChangeLog:
            PR target/96795
            PR target/107515
            * config/arm/arm_mve.h (__ARM_mve_coerce2): Split types.
            (__ARM_mve_coerce3): Likewise.
    
    gcc/testsuite/ChangeLog:
            PR target/96795
            PR target/107515
            * gcc.target/arm/mve/intrinsics/mve_intrinsic_type_overloads-fp.c: New test.
            * gcc.target/arm/mve/intrinsics/mve_intrinsic_type_overloads-int.c: New test.
Comment 11 GCC Commits 2023-05-18 10:39:25 UTC
The releases/gcc-12 branch has been updated by Stam Markianos-Wright <stammark@gcc.gnu.org>:

https://gcc.gnu.org/g:79431d45e2c25dc8608c8ddc6d2798dbcb79650a

commit r12-9559-g79431d45e2c25dc8608c8ddc6d2798dbcb79650a
Author: Stam Markianos-Wright <stam.markianos-wright@arm.com>
Date:   Thu Nov 10 15:06:47 2022 +0000

    arm: Explicitly specify other float types for _Generic overloading [PR107515]
    
    This patch adds explicit references to other float types
    to __ARM_mve_typeid in arm_mve.h.  Resolves PR 107515:
    https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107515
    
    gcc/ChangeLog:
            PR target/107515
            * config/arm/arm_mve.h (__ARM_mve_typeid): Add float types.
    
    (cherry picked from commit 2fefb8931d566cc8a4cbba81601972b0d2002f95)
Comment 12 GCC Commits 2023-05-18 10:43:37 UTC
The releases/gcc-12 branch has been updated by Stam Markianos-Wright <stammark@gcc.gnu.org>:

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

commit r12-9608-gc7c4dfb5989e80ffc8e8439a8d9a9ed654612b90
Author: Stam Markianos-Wright <stam.markianos-wright@arm.com>
Date:   Mon Jan 16 11:40:40 2023 +0000

    arm: Split up MVE _Generic associations to prevent type clashes [PR107515]
    
    With these previous patches:
    https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606586.html
    https://gcc.gnu.org/pipermail/gcc-patches/2022-November/606587.html
    we enabled the MVE overloaded _Generic associations to handle more
    scalar types, however at PR 107515 we found a new regression that
    wasn't detected in our testing:
    
    With glibc's posix/types.h:
    ```
    typedef signed int __int32_t;
    ...
    typedef __int32_t int32_t;
    ```
    We would get a `error: '_Generic' specifies two compatible types`
    from `__ARM_mve_coerce3` because of `type: param`, when `type` is
    `int` and `int32_t: param` both being the same under the hood.
    
    The same did not happen with Newlib's header sys/_stdint.h:
    ```
    typedef long int __int32_t;
    ...
    typedef __int32_t int32_t ;
    ```
    which worked fine, because it uses `long int`.
    
    The same could feasibly happen in `__ARM_mve_coerce2` between
    `__fp16` and `float16_t`.
    
    The solution here is to break the _Generic down so that the similar
    types don't appear at the same level, as is done in `__ARM_mve_typeid`
    
    gcc/ChangeLog:
            PR target/96795
            PR target/107515
            * config/arm/arm_mve.h (__ARM_mve_coerce2): Split types.
            (__ARM_mve_coerce3): Likewise.
    
    gcc/testsuite/ChangeLog:
            PR target/96795
            PR target/107515
            * gcc.target/arm/mve/intrinsics/mve_intrinsic_type_overloads-fp.c: New test.
            * gcc.target/arm/mve/intrinsics/mve_intrinsic_type_overloads-int.c: New test.
    
    (cherry picked from commit 8a1360e72d6c6056606aa5edd8c906c50f26de59)
Comment 13 Stam Markianos-Wright 2023-05-18 11:06:55 UTC
Fixed on GCC12 onwards.