Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][LoongArch] LSX/LASX headers do not work with -fno-lax-vector-conversions #110834

Closed
xen0n opened this issue Oct 2, 2024 · 10 comments · Fixed by #114513
Closed

[clang][LoongArch] LSX/LASX headers do not work with -fno-lax-vector-conversions #110834

xen0n opened this issue Oct 2, 2024 · 10 comments · Fixed by #114513
Labels
backend:loongarch clang:headers Headers provided by Clang, e.g. for intrinsics release:backport

Comments

@xen0n
Copy link
Contributor

xen0n commented Oct 2, 2024

The current Clang LSX/LASX intrinsics header files make heavy use of implicit vector conversions vector bitcasts, that is not accepted by Clang 18+ (Clang 17 does not support Loongson SIMD) with -fno-lax-vector-conversions. Example:

// clang -c -fno-lax-vector-conversions -mlsx test-lsx-lax-conversion.c
#include <lsxintrin.h>
int main(void) { return 0; }
In file included from test-lsx-lax-conversion.c:1:
/usr/lib/llvm/19/bin/../../../../lib/clang/19/include/lsxintrin.h:42:40: error: cannot initialize a parameter of type '__attribute__((__vector_size__(16 * sizeof(char)))) char' (vector of 16 'char' values) with an rvalue of type 'v16i8' (vector of 16 'signed char' values)
   42 |   return (__m128i)__builtin_lsx_vsll_b((v16i8)_1, (v16i8)_2);
      |                                        ^~~~~~~~~
[lots of similar errors omitted]

It complicates distribution packaging for certain software, because

  1. they require or work better with Clang, and
  2. effectively one is forced to enable -flax-vector-conversions here and there, for example Skia.

So it may be best to just fix the headers to work with strict vector conversion checks, to stop the proliferation of lax coding practice.

@github-actions github-actions bot added the clang Clang issues not falling into any other category label Oct 2, 2024
@EugeneZelenko EugeneZelenko added clang:headers Headers provided by Clang, e.g. for intrinsics backend:loongarch and removed clang Clang issues not falling into any other category labels Oct 2, 2024
@xen0n
Copy link
Contributor Author

xen0n commented Oct 3, 2024

Sketch: https://godbolt.org/z/s94GWb3fs

Sketch code
// compile with: -O1 -mlsx -fno-lax-vector-conversions
// for Clang, add --target=loongarch64 if not natively experimenting

#ifdef __clang__
// #include <lsxintrin.h>
typedef signed char v16i8 __attribute__((vector_size(16), aligned(16)));
typedef long long __m128i __attribute__((__vector_size__(16), __may_alias__));

// clang distinguishes between "signed char" and "char"
// __builtin_lsx_vsll_b is defined with the following:
//
//     TARGET_BUILTIN(__builtin_lsx_vsll_b, "V16cV16cV16c", "nc", "lsx")
//
// so it doesn't accept "signed char" vectors
// (each "V16c" would have to become "V16Sc" for "signed char")
//
// kludge without modifying Clang: provide a v16i8 defined with bare "char"
typedef char v16c8 __attribute__((vector_size(16), aligned(16)));
#else
#include <lsxintrin.h>
typedef v16i8 v16c8;
#endif

__m128i __lsx_vsll_b_xxx(__m128i _1, __m128i _2) {
  // return (__m128i)__builtin_lsx_vsll_b((v16i8)_1, (v16i8)_2);
  return (__m128i)__builtin_lsx_vsll_b((v16c8)_1, (v16c8)_2);
}

We probably have to provide a bare char variant of v16i8, or amend the signature of possibly many builtins involving chars to have signedness always explicitly spelled out.

@xen0n
Copy link
Contributor Author

xen0n commented Oct 31, 2024

CC-ing people for better visibility, this is best handled ASAP because toolchain changes can take a long time to propagate to distros:

cc @ChenghuaXu @chenglulu326 @SixWeining @heiher @xry111 (toolchain)
cc @KexyBiscuit @wszqkzqk (packagers)
cc @double1Kai (LoongArch skia)

@xry111
Copy link
Contributor

xry111 commented Oct 31, 2024

AFAIK removing "signed" is not an option because it'll cause stupid things if someone uses -funsigned-char.

@xry111
Copy link
Contributor

xry111 commented Oct 31, 2024

Thus the correct thing to do is fixing clang builtin to accept a signed char vector.

xry111 added a commit to xry111/llvm-project that referenced this issue Oct 31, 2024
…rsions=none`

Change all `V16c` and `V32c` to `V16Sc` and `V32Sc`, and fix some other
random inconsistencies between the builtins and the intrinsic headers.
Add tests two ensure the intrinsic headers work with
`-flax-vector-conversions=none`.

Fixes llvm#110834.
xry111 added a commit to xry111/llvm-project that referenced this issue Oct 31, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
@wangleiat
Copy link
Contributor

Thus the correct thing to do is fixing clang builtin to accept a signed char vector.

I completely agree with your point.

@xen0n
Copy link
Contributor Author

xen0n commented Nov 1, 2024

Previously I worried about compatibility of signatures of the builtins, but @xry111 told me yesterday that the public API surface is the intrinsic header and not the builtins. And with the immediate errors on the inclusion of the header, there's no previously-working code that would be regressed by the signature change either.

So I would also agree that amending the builtins' signatures is the best way forward.

xry111 added a commit to xry111/llvm-project that referenced this issue Nov 1, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 1, 2024
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 1, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 1, 2024
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 1, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 1, 2024
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 1, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 2, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
SixWeining pushed a commit that referenced this issue Nov 2, 2024
…s for LSX and LASX builtins (#114510)

`-flax-vector-conversions=none` does not allow an implicit conversion
from `signed char` vector to `char` vector, and we cannot remove
`signed`
from `v16i8` or `v32i8` because doing so will break our expectation with
`-fno-signed-char`.  So to make lsxintrin.h and lasxintrin.h work fine
with `-flax-vector-conversions=none`, we must use `signed char` instead
of `char`.
    
The change is just done via
    
    sed 's/V16c/V16Sc/g' -i BuiltinsLoongArchLSX.def
    sed 's/V32c/V32Sc/g' -i BuiltinsLoongArchLASX.def

Depends on #114509.  Part of #110834 fix.
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 2, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
SixWeining pushed a commit that referenced this issue Nov 2, 2024
…b builtins `signed char` vector (#114511)

These builtins operate on int8 vectors, not int16 vectors.  So the old
definition does not make any sense.

Depends on #114510.  Part of #110834 fix.
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 2, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
smallp-o-p pushed a commit to smallp-o-p/llvm-project that referenced this issue Nov 3, 2024
xry111 added a commit to xry111/llvm-project that referenced this issue Nov 4, 2024
… builtins `unsigned char` vectors

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.
@xen0n
Copy link
Contributor Author

xen0n commented Nov 5, 2024

@SixWeining @xry111 -- Thanks for the fix and quick review & merge; I think it may also be appropriate to backport this patchset to LLVM 19.x because this would allow downstream to drop their kludges 6 months earlier.

(We have to act quick though, LLVM 19.1.4 is slated for Nov 12th i.e. a week from now. I cannot help this time due to very tight schedule though.)

@SixWeining
Copy link
Contributor

@SixWeining @xry111 -- Thanks for the fix and quick review & merge; I think it may also be appropriate to backport this patchset to LLVM 19.x because this would allow downstream to drop their kludges 6 months earlier.

Yes, I think so.

@SixWeining
Copy link
Contributor

/cherry-pick 96d2196 b885054 92daad2 4006b28 4f740f9

@llvmbot
Copy link
Member

llvmbot commented Nov 5, 2024

/pull-request #114958

PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this issue Nov 6, 2024
…b builtins `signed char` vectors (llvm#114512)

The lsxintrin.h and and lasxintrin.h headers uses `signed char` vectors
instead of `unsigned char` vectors.  GCC also uses `signed char` for
them, so align their definition with the headers and GCC.

Depends on llvm#114511.  Part of llvm#110834 fix.
PhilippRados pushed a commit to PhilippRados/llvm-project that referenced this issue Nov 6, 2024
… builti ns `unsigned char` vectors (llvm#114513)

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.
    
Fixes llvm#110834.

Depends on llvm#114512.
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 12, 2024
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 12, 2024
…s for LSX and LASX builtins (llvm#114510)

`-flax-vector-conversions=none` does not allow an implicit conversion
from `signed char` vector to `char` vector, and we cannot remove
`signed`
from `v16i8` or `v32i8` because doing so will break our expectation with
`-fno-signed-char`.  So to make lsxintrin.h and lasxintrin.h work fine
with `-flax-vector-conversions=none`, we must use `signed char` instead
of `char`.

The change is just done via

    sed 's/V16c/V16Sc/g' -i BuiltinsLoongArchLSX.def
    sed 's/V32c/V32Sc/g' -i BuiltinsLoongArchLASX.def

Depends on llvm#114509.  Part of llvm#110834 fix.

(cherry picked from commit b885054)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 12, 2024
…b builtins `signed char` vector (llvm#114511)

These builtins operate on int8 vectors, not int16 vectors.  So the old
definition does not make any sense.

Depends on llvm#114510.  Part of llvm#110834 fix.

(cherry picked from commit 92daad2)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 12, 2024
…b builtins `signed char` vectors (llvm#114512)

The lsxintrin.h and and lasxintrin.h headers uses `signed char` vectors
instead of `unsigned char` vectors.  GCC also uses `signed char` for
them, so align their definition with the headers and GCC.

Depends on llvm#114511.  Part of llvm#110834 fix.

(cherry picked from commit 4006b28)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 12, 2024
… builti ns `unsigned char` vectors (llvm#114513)

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.

Depends on llvm#114512.

(cherry picked from commit 4f740f9)
@tru tru moved this from Needs Triage to Done in LLVM Release Status Nov 12, 2024
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 15, 2024
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 15, 2024
…s for LSX and LASX builtins (llvm#114510)

`-flax-vector-conversions=none` does not allow an implicit conversion
from `signed char` vector to `char` vector, and we cannot remove
`signed`
from `v16i8` or `v32i8` because doing so will break our expectation with
`-fno-signed-char`.  So to make lsxintrin.h and lasxintrin.h work fine
with `-flax-vector-conversions=none`, we must use `signed char` instead
of `char`.

The change is just done via

    sed 's/V16c/V16Sc/g' -i BuiltinsLoongArchLSX.def
    sed 's/V32c/V32Sc/g' -i BuiltinsLoongArchLASX.def

Depends on llvm#114509.  Part of llvm#110834 fix.

(cherry picked from commit b885054)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 15, 2024
…b builtins `signed char` vector (llvm#114511)

These builtins operate on int8 vectors, not int16 vectors.  So the old
definition does not make any sense.

Depends on llvm#114510.  Part of llvm#110834 fix.

(cherry picked from commit 92daad2)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 15, 2024
…b builtins `signed char` vectors (llvm#114512)

The lsxintrin.h and and lasxintrin.h headers uses `signed char` vectors
instead of `unsigned char` vectors.  GCC also uses `signed char` for
them, so align their definition with the headers and GCC.

Depends on llvm#114511.  Part of llvm#110834 fix.

(cherry picked from commit 4006b28)
tru pushed a commit to llvmbot/llvm-project that referenced this issue Nov 15, 2024
… builti ns `unsigned char` vectors (llvm#114513)

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.

Depends on llvm#114512.

(cherry picked from commit 4f740f9)
nikic pushed a commit to rust-lang/llvm-project that referenced this issue Nov 20, 2024
nikic pushed a commit to rust-lang/llvm-project that referenced this issue Nov 20, 2024
…s for LSX and LASX builtins (llvm#114510)

`-flax-vector-conversions=none` does not allow an implicit conversion
from `signed char` vector to `char` vector, and we cannot remove
`signed`
from `v16i8` or `v32i8` because doing so will break our expectation with
`-fno-signed-char`.  So to make lsxintrin.h and lasxintrin.h work fine
with `-flax-vector-conversions=none`, we must use `signed char` instead
of `char`.

The change is just done via

    sed 's/V16c/V16Sc/g' -i BuiltinsLoongArchLSX.def
    sed 's/V32c/V32Sc/g' -i BuiltinsLoongArchLASX.def

Depends on llvm#114509.  Part of llvm#110834 fix.

(cherry picked from commit b885054)
nikic pushed a commit to rust-lang/llvm-project that referenced this issue Nov 20, 2024
…b builtins `signed char` vector (llvm#114511)

These builtins operate on int8 vectors, not int16 vectors.  So the old
definition does not make any sense.

Depends on llvm#114510.  Part of llvm#110834 fix.

(cherry picked from commit 92daad2)
nikic pushed a commit to rust-lang/llvm-project that referenced this issue Nov 20, 2024
…b builtins `signed char` vectors (llvm#114512)

The lsxintrin.h and and lasxintrin.h headers uses `signed char` vectors
instead of `unsigned char` vectors.  GCC also uses `signed char` for
them, so align their definition with the headers and GCC.

Depends on llvm#114511.  Part of llvm#110834 fix.

(cherry picked from commit 4006b28)
nikic pushed a commit to rust-lang/llvm-project that referenced this issue Nov 20, 2024
… builti ns `unsigned char` vectors (llvm#114513)

The lsxintrin.h and and lasxintrin.h headers uses `unsigned char`
vectors instead of `signed char` vectors.  GCC also uses `unsigned char`
for them, so align their definition with the headers and GCC.

Fixes llvm#110834.

Depends on llvm#114512.

(cherry picked from commit 4f740f9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:loongarch clang:headers Headers provided by Clang, e.g. for intrinsics release:backport
Projects
6 participants