linux 🚀 - -Wincompatible-pointer-types in arch/arm64/lib/xor-neon.c

Might be good to see more of vld1q_u64. Maybe that function should take unsigned long * rather than uint64_t *?

Shot in the dark; Clang might expect a const unsigned long * for its __builtin_neon_vld1q_v but gcc might expect unsigned long long *. Or gcc doesn't have this warning.

nickdesaulniers on 9 Dec 2018

Looks like vld1q_u64 is defined in the compiler headers, not the kernel.

Clang (not sure how it gets generated because I didn't find vld1q_u64 in the source):

#ifdef __LITTLE_ENDIAN__
#define vld1q_u64(__p0) __extension__ ({ \
  uint64x2_t __ret; \
  __ret = (uint64x2_t) __builtin_neon_vld1q_v(__p0, 51); \
  __ret; \
})
#else
#define vld1q_u64(__p0) __extension__ ({ \
  uint64x2_t __ret; \
  __ret = (uint64x2_t) __builtin_neon_vld1q_v(__p0, 51); \
  __ret = __builtin_shufflevector(__ret, __ret, 1, 0); \
  __ret; \
})
#endif

gcc/config/aarch64/arm_neon.h:

__extension__ extern __inline uint64x2_t
__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
vld1q_u64 (const uint64_t *a)
{   
  return (uint64x2_t)
    __builtin_aarch64_ld1v2di ((const __builtin_aarch64_simd_di *) a);
}

This might sound like a stupid question, when I search for __builtin_neon_vld1q_v within Clang, I don't see any function, rather just a couple of references in lib/CodeGen/CGBuiltin.cpp. How would I go about seeing what the expected parameters to that builtin are?

nathanchance on 9 Dec 2018

cc @smithp35

CONFIG_XOR_BLOCKS Seeing this now in mainline.

nickdesaulniers on 7 Jan 2019

@nathanchance how come our CI isn't reporting this failure? I see it locally for an arm64 defconfig. Is it that our CI's clang is outdated (and thus a regression on the Clang side)?

nickdesaulniers on 14 Jan 2019

Ah, CONFIG_XOR_BLOCKS gets selected as a module, which we aren't building on CI. Maybe we should build modules as a separate build target for each build. It would double our matrix size but we would get more coverage.

nathanchance on 14 Jan 2019

Hi @nathanchance,

today I stepped on the same problem described in this thread (Linux 5.0-rc2 and clang 8) and after a bit of debugging seems that it is triggered because the arm_neon builtins input parameters are not compliant with what described in ACLE (Arm C Language Extentions): http://infocenter.arm.com/help/topic/com.arm.doc.ecm0665619/acle_sve_100987_0000_00_en.pdf.

I came to this conclusion because if we take for example __builtin_neon_vld1q_v that is defined as:
arm_neon.inc:299 BUILTIN(__builtin_neon_vld1q_v, "V16ScvC*i", "n")
and decode the string V16ScvC*i (based on what described in Builtins.def) we can clearly see that the builtin is expected to return a vector of size 16 (V16) of signed char (Sc) and to take as input a "const void *" (vC*) and an integer (i).

To be compliant with the ACLE the vC* parameter should become UWi (uint64_t).

All the other compilation issues share similar reason. Please let me know what do you think.

Thanks!

fvincenzo on 16 Jan 2019

@fvincenzo Thanks for the insight!

It looks like that arm_neon.inc file gets generated by clang/utils/TableGen/NeonEmitter.cpp in combination with clang/include/clang/Basic/arm_neon.td, if I am following everything correctly. The parameters you talk about get generated by getBuiltinTypeStr and builtin_str I think.

Have you created a patch yet to change the generation of the arm_neon.inc file? I'm trying to look into it right now but multiple eyes never hurts.

nathanchance on 16 Jan 2019

@nathanchance I did not create a patch to target the issue yet. It took me a while to figure out the problem. If you created it in the meantime I am happy to review it.

Thanks.

fvincenzo on 17 Jan 2019

@fvincenzo I won't be free to seriously look into creating a fix until Monday so if you can beat me to it in that time, please do (especially since you have a basic understanding of the problem).

nathanchance on 17 Jan 2019

I can help write a patch (thanks @fvincenzo for the detailed report!).

nickdesaulniers on 17 Jan 2019

Hi,
I'm currently fixing a similar issue in clang, https://reviews.llvm.org/D56852 would give you an idea of what the patch would look like. As @fvincenzo says, the fix comes from changing the string to describe the types.

sparker-arm on 18 Jan 2019

Thanks for the tips. I think the vld1q variants are described in:

include/clang/Basic/arm_neon.td:

 348 def VLD1      : WInst<"vld1", "dc",                                             
 349                       "QUcQUsQUiQUlQcQsQiQlQfQPcQPsUcUsUiUlcsilfPcPs">;

and can see a breakdown of these codes in include/clang/Basic/arm_neon_incl.td, but I can't say I understand this big long string. Is it defining multiple prototypes at once? How does this work?

nickdesaulniers on 18 Jan 2019

These strings are a mad riddle to me, the second string declares a prototype that gets mutated in some way by the third string. But after looking at this a bit closer, I don't think it's where this problem can be solved though. The intrinsics just use void pointers and then there's some manual semantic checking that happens, which is where this warning is coming from (in clang/lib/SemaChecking.cpp Sema::CheckNeonBuiltinFunctionaCall). The variable IsInt64Long is set to true and so we generate an unsigned long.

sparker-arm on 21 Jan 2019

@sparker-arm I agree with your analisys, I came to similar conclusion. According to me IsInt64Long should not be checked in this case, because ACLE defines the types of the arm_neon builtins input parameters as uint64_t (unsigned long long) hence unless I missed something NeonTypeFlags::Int64 and NeonTypeFlags::Poly64 should always return Signed/Unsigned Long Long.

@nathanchance @nickdesaulniers below the fix that made me build the kernel correctly (Might not be complete, hence I am not submitting a patch):

diff --git a/lib/Sema/SemaChecking.cpp b/lib/Sema/SemaChecking.cpp
index 0598da214e..3f8aec1b69 100644
--- a/lib/Sema/SemaChecking.cpp
+++ b/lib/Sema/SemaChecking.cpp
@@ -1526,9 +1526,6 @@ static QualType getNeonEltType(NeonTypeFlags Flags, ASTContext &Context,
   case NeonTypeFlags::Int32:
     return Flags.isUnsigned() ? Context.UnsignedIntTy : Context.IntTy;
   case NeonTypeFlags::Int64:
-    if (IsInt64Long)
-      return Flags.isUnsigned() ? Context.UnsignedLongTy : Context.LongTy;
-    else
       return Flags.isUnsigned() ? Context.UnsignedLongLongTy
                                 : Context.LongLongTy;
   case NeonTypeFlags::Poly8:
@@ -1536,9 +1533,6 @@ static QualType getNeonEltType(NeonTypeFlags Flags, ASTContext &Context,
   case NeonTypeFlags::Poly16:
     return IsPolyUnsigned ? Context.UnsignedShortTy : Context.ShortTy;
   case NeonTypeFlags::Poly64:
-    if (IsInt64Long)
-      return Context.UnsignedLongTy;
-    else
       return Context.UnsignedLongLongTy;
   case NeonTypeFlags::Poly128:
     break;

fvincenzo on 21 Jan 2019

Trying to test a patch for this has been a pain because I can't seem to hit the issue when using the clang test driver. Passing -ffreestanding to clang appears to alleviate these stdint issues, does this work for you guys too?

sparker-arm on 21 Jan 2019

@sparker-arm This translation unit already uses that flag: https://github.com/ClangBuiltLinux/linux/blob/cc9f8349cb33965120a96c12e05d00676162eb7f/arch/arm64/lib/Makefile#L11

@fvincenzo I will test that patch now.

Would the sema checking need to be modified if the definitions in arm_neon.inc were generated with the correct prototype? Something tells me that's the proper solution but I have no idea how to change the couple of functions I mentioned above to do it.

nathanchance on 21 Jan 2019

Thanks for pointing that out @nathanchance
But I think there's still something odd going on here, the code in the compiler looks sane to me and it just seems there's a mismatch between a typedef sitting in a header somewhere. I took the xor-neon.c file, removed the headers and just included arm_neon.h instead and the freestanding option then removes all warnings.

sparker-arm on 22 Jan 2019

The code @fvincenzo commented on was modified by commits 7e0e8ef787107 ("ARM64: initial clang support commit.") r205100, 491dd1c4d1261 ("[AArch64] Change int64_t from 'long long int' to 'long int' for AArch64 target.") r202004, d13c501b4c9d9 ("[AArch64 ACLE] Allow to define poly64_t as 'unsigned long long' on LLP64 system.") r237348.

491dd1c4d1261 ("[AArch64] Change int64_t from 'long long int' to 'long int' for AArch64 target.") r202004

looks the most interesting. If I make those changes @fvincenzo suggests, the following llvm tests fail:

CodeGenCXX/aarch64-neon.cpp (only for the poly64x1_t case)

I will contact the author of r202004.