[PATCH] D93585: [AArch64] Enable out-of-line atomics by default.
jgreenhalgh added inline comments. Comment at: llvm/lib/Target/AArch64/AArch64.td:1087 FeatureNEON, + FeatureOutlineAtomics, FeaturePerfMon, t.p.northover wrote: > ilinpv wrote: > > t.p.northover wrote: > > > I think this still enables it more widely than we want. Clang overrides > > > it with `-outline-atomics`, but other front-ends don't. > > Could I ask you to clarify what front-ends you meant (to check outline > > atomics suport for them)? > Any front-end that generates LLVM IR. Generally important ones are Swift and > Rust, but there are many more and I kind of doubt it's feasible to ensure > they all will. It's unfortunate, and maybe at some point we can put logic in > LLVM to approximate Clang's decision-making and get the benefit without any > opt-in, but I kind of think it's a bit early yet. We'd risk breaking too many > people's builds. > > Also, putting it in the generic CPU model here means that if a front-end > allows something like `-mcpu=cortex-a57` it will suddenly disable outlined > atomics. Whether they're good or bad, that's just plain weird behaviour. A > CPU-level feature isn't the right place for this. Not necessarily arguing in either direction for where these should be enabled, but is that behaviour really so weird? I would expect that we want to specialise to inline atomics as soon as we're given architecture permission (-march=armv8.1-a+lse or greater) or CPU-directed specialisation. These things are only useful for the transition case where your platform mandates Armv8.0 only binaries but you're expecting to also run fast on cores with LSE support. What we're nervous about is burying this option, which we believe to be generally useful for the Armv8.0 platforms, such that it is opt-in. It really isn't the end of the world if this is C/C++ on LInux/Android only for now, but it feels like missed opportunity. That said, it is safe, so probably makes sense to go for that now Pavel and think again about where else we want it on. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93585/new/ https://reviews.llvm.org/D93585 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D51683: Fix arm_neon.h and arm_fp16.h generation for compiling with std=c89
jgreenhalgh added inline comments. Comment at: cfe/trunk/utils/TableGen/NeonEmitter.cpp:2412 - OS << "#define __ai static inline __attribute__((__always_inline__, " + OS << "#define __ai static __inline __attribute__((__always_inline__, " "__nodebug__))\n\n"; dnsampaio wrote: > joerg wrote: > > If you want to change it, at least change it properly to use __inline__. > Sorry, I don't get the suggestion. Do you mean test if it is C89 and use > __inline, else, use inline? I think the suggestion was unintentionally rendered as an underline by phab; and was supposed to be ``` __inline__ ``` https://reviews.llvm.org/D51683 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D45668: [NEON] Define vget_high_f16() and vget_low_f16() intrinsics in AArch64 mode only
jgreenhalgh added a comment. In https://reviews.llvm.org/D45668#1070878, @SjoerdMeijer wrote: > Thanks, and I am going to try to get some clarity on this doc issue. But > looks like it should be "ARMv7, ARMv8", as it used to be. Make sense to > comment on this in the commit message, if that's what you mean. These should be available whenever the float16x4_t and float16x8_t types are available. So v7/A32/A64. I have pushed this change to the docs locally; but I don't know when this will make it to public documentation, so you will just have to take my word for it when I say this is a documentation bug and it will be fixed in a future release. Repository: rL LLVM https://reviews.llvm.org/D45668 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D40673: Add _Float128 as alias to __float128 to enable compilations on Fedora27/glibc2-26
jgreenhalgh added a comment. If this patch unconditionally defines _Float128, then I think it will conflict with the typedef for _Float128 for IEEE754 128-bit long double systems in glibc: /* The type _Float128 exists only since GCC 7.0. */ # if !__GNUC_PREREQ (7, 0) || defined __cplusplus typedef long double _Float128; # endif https://sourceware.org/git/?p=glibc.git;a=blob_plain;f=sysdeps/ieee754/ldbl-128/bits/floatn.h;hb=HEAD This manifests on AArch64 as compile time failure as so: $ cat x.c #include $ clang x.c In file included from x.c:1: In file included from /usr/include/stdlib.h:55: /usr/include/aarch64-linux-gnu/bits/floatn.h:67:21: error: cannot combine with previous 'double' declaration specifier typedef long double _Float128; ^ 1 error generated. I think that is an inadequate guard check in glibc, but perhaps there is something clang can do to help out? Thanks, James Repository: rC Clang https://reviews.llvm.org/D40673 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
