This is an archive of the discontinued LLVM Phabricator instance.

[clang][BFloat] Avoid redefining bfloat16_t in arm_neon.h
ClosedPublic

Authored by dim on Apr 20 2023, 11:12 AM.

Details

Summary

As of https://reviews.llvm.org/D79708, clang-tblgen generates arm_neon.h,
arm_sve.h and arm_bf16.h, and all those generated files will contain a
typedef of bfloat16_t. However, arm_neon.h and arm_sve.h include
arm_bf16.h immediately before their own typedef:

#include <arm_bf16.h>
typedef __bf16 bfloat16_t;

With a recent version of clang (I used 16.0.1) this results in warnings:

/usr/lib/clang/16/include/arm_neon.h:38:16: error: redefinition of typedef 'bfloat16_t' is a C11 feature [-Werror,-Wtypedef-redefinition]

Since arm_bf16.h is very likely supposed to be the one true place where
bfloat16_t is defined, I propose to delete the duplicate typedefs from the
generated arm_neon.h and arm_sve.h.

Diff Detail

Event Timeline

dim created this revision.Apr 20 2023, 11:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 11:12 AM
dim requested review of this revision.Apr 20 2023, 11:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2023, 11:12 AM
dim updated this revision to Diff 515409.Apr 20 2023, 11:32 AM

Also fix up arm_sve.h.

dim edited the summary of this revision. (Show Details)Apr 20 2023, 11:33 AM

I can't see anything wrong with this patch and it looks pretty straightforward to me, but is it necessary?

'-Wtypedef-redefinition' is documented as not applying to system headers, and I couldn't reproduce the issue unless I changed the headers to not be system headers.

dim added a comment.Apr 28 2023, 8:00 AM

I can't see anything wrong with this patch and it looks pretty straightforward to me, but is it necessary?

'-Wtypedef-redefinition' is documented as not applying to system headers, and I couldn't reproduce the issue unless I changed the headers to not be system headers.

I think this due to the way some parts of FreeBSD get compiled, where we definitely use -Wsystem-headers. It is likely that this warning (which was turned into an error because parts of the tree also get compiled with -Werror) came through because of that. In any case, there is no need to redefine bfloat16_t, at least not here. Another option is to add yet another define like __bfloat_16_defined and test for that, but it only makes things uglier.

sdesmalen accepted this revision.May 2 2023, 1:27 AM

I agree it makes sense to remove the typedef if they are also defined in arm_bf16.h.

This revision is now accepted and ready to land.May 2 2023, 1:27 AM
simonbutcher accepted this revision.May 2 2023, 3:53 AM

we definitely use -Wsystem-headers.

Fair enough, that hadn't occurred to me. The change LGTM.

This revision was landed with ongoing or failed builds.May 3 2023, 8:55 AM
This revision was automatically updated to reflect the committed changes.