[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added inline comments. Comment at: compiler-rt/lib/builtins/cpu_model.c:1338 + hwcap = getauxval(AT_HWCAP); + hwcap2 = getauxval(AT_HWCAP2); +#endif // defined(__FreeBSD__) nikic wrote: > This breaks the build with glibc 2.17. Thanks for

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

nikic added inline comments. Comment at: compiler-rt/lib/builtins/cpu_model.c:1338 + hwcap = getauxval(AT_HWCAP); + hwcap2 = getauxval(AT_HWCAP2); +#endif // defined(__FreeBSD__) This breaks the build with glibc 2.17. Repository: rG LLVM Github Monorepo

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4085171 , @tmatheson wrote: > This patch has made it considerably harder to understand what is going on in > the TargetParser. If you get a chance, please could you add some clarifying > comments and tidy-ups. I

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

tmatheson added a comment. This patch has made it considerably harder to understand what is going on in the TargetParser. If you get a chance, please could you add some clarifying comments and tidy-ups. I appreciate that a lot of this is following the lead of the pre-existing TargetParser

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4031994 , @smeenai wrote: > You're right that it conceptually makes sense for this to be in `cpu_model.c` > though. An alternative would be providing an option for compiler-rt to be > built without the multiversioning

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

smeenai added a comment. In D127812#4031849 , @ilinpv wrote: > In D127812#4031313 , @smeenai wrote: > >> We can use `-mno-fmv` to avoid that dependency, right? We're interested in >> using that for our own code

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4031313 , @smeenai wrote: > We can use `-mno-fmv` to avoid that dependency, right? We're interested in > using that for our own code (where we don't make use of function > multi-versioning), and want to prevent the

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

smeenai added a comment. In D127812#4031101 , @ilinpv wrote: > In D127812#4030881 , @smeenai wrote: > >> We're not actually using multi-versioning anywhere, but we're still paying >> the size cost for it as a

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4030881 , @smeenai wrote: > We're not actually using multi-versioning anywhere, but we're still paying > the size cost for it as a result. Would we consider moving the newly added > functions into their own file (or

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

smeenai added subscribers: yozhu, lanza, smeenai. smeenai added a comment. I'm investigating a size increase we observed after this change for Meta's Android apps. This increases the size of compiler-rt by 1.6 KB, which is small by itself, but then compiler-rt is statically linked into each SO,

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4018490 , @mceier wrote: > Checked the changes I'm suggesting and they fix the standalone build. Thanks! Fix committed 2184fcf17ee00a939b3bde98a28ef586c67d6b1a

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

mceier added a comment. Checked the changes I'm suggesting and they fix the standalone build. Comment at: clang/lib/Basic/Targets/AArch64.cpp:650 + .Case(NAME, FMV_PRIORITY) +#include "../../../../llvm/include/llvm/TargetParser/AArch64TargetParser.def" + ;

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

hctim added a comment. In D127812#4012854 , @ilinpv wrote: > I don't think `std::equal` is underlying cause here. We have > featuresStrs_size() comparison before calling it: > > if (CurClones && NewClones && >

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4012283 , @hctim wrote: > I'm not sure "MSan didn't handle correctly SmallVector" is the case. Given > your diagnosis of 3-elements-vs-2, I'm guessing the root cause is that > `clang/lib/Sema/SemaDecl.cpp:11369` is

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

MaskRay added a comment. In D127812#4011437 , @hctim wrote: > LDFLAGS="$LDFLAGS -Wl,--rpath=/tmp/2/lib" # < use the instrumented libcxx > from step 2 I think -Wl,-rpath and -isystem are somewhat different.

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

hctim added a comment. In D127812#4012276 , @ilinpv wrote: > I've managed to reproduce "MemorySanitizer: use-of-uninitialized-value" error > locally, thank you @hctim for help! > If I understand it right, it seems **MSan didn't handle correctly >

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. I've managed to reproduce "MemorySanitizer: use-of-uninitialized-value" error locally, thank you @hctim for help! If I understand it right, it seems **MSan didn't handle correctly SmallVector** - a variable-sized array with some number of elements in-place and heap

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4011437 , @hctim wrote: > 2. Build a sanitizer libcxx. > > $ cd /tmp/2 > $ cmake \ > -DCMAKE_C_COMPILER=/tmp/1/bin/clang \ > -DCMAKE_CXX_COMPILER=/tmp/1/bin/clang++ \ > -GNinja \ >

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

hctim added a comment. In D127812#4011372 , @ilinpv wrote: > Regular builds works fine for me, pthreads located here > "/lib/x86_64-linux-gnu/libpthread.so" > "/usr/lib/x86_64-linux-gnu/libpthread.so". Enabling > "-DLLVM_USE_SANITIZER=Memory" resulted

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4010993 , @hctim wrote: > Hmm, not exactly sure what's going on with the `could NOT find Threads` > there. A quick googling seems to point to pthreads.so not being in the right > places, but I don't think the buildbot

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

hctim added a comment. In D127812#4010856 , @ilinpv wrote: > It would be great to have more details how to setup up your bot, using > buildbot_fast.sh on x86_64 Ubuntu 22.04 LTS leads to error ( pthreads > installed ): > > CMake Error at >

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4009577 , @hctim wrote: > In D127812#4009447 , @hctim wrote: > >> Hi, this looks like a candidate for breaking the MSan bot: >>

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

hctim added a comment. In D127812#4009447 , @hctim wrote: > Hi, this looks like a candidate for breaking the MSan bot: > https://lab.llvm.org/buildbot/#/builders/5/builds/30139 > > Still looking into it and bisecting, will let you know when I have more

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

hctim added a comment. Hi, this looks like a candidate for breaking the MSan bot: https://lab.llvm.org/buildbot/#/builders/5/builds/30139 Still looking into it and bisecting, will let you know when I have more info. To reproduce the bots, the best way (because MSan setup is tricky because it

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

paulkirth added a comment. Thanks! I think we're good now. At least one of our builders is green now. https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8794218564332891457/overview I'll let you know if anything pops up, but this looks squared away to me. Repository:

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4009014 , @paulkirth wrote: > Hi, thanks for the fix. that unblocked our builder. Unfortunately, we still > see some errors in tests. > > FAIL: Clang :: Driver/aarch64-features.c (7460 of 16622) >

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

paulkirth added a comment. Hi, thanks for the fix. that unblocked our builder. Unfortunately, we still see some errors in tests. FAIL: Clang :: Driver/aarch64-features.c (7460 of 16622) TEST 'Clang :: Driver/aarch64-features.c' FAILED Script:

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. In D127812#4008451 , @paulkirth wrote: > Hi, we're seeing a build failure in Fuchsia's Clang CI. We're seeing this on > all of our builders: arm64 & x64 linux, mac and windows > > FAILED:

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

paulkirth added a comment. Hi, we're seeing a build failure in Fuchsia's Clang CI. We're seeing this on all of our builders: arm64 & x64 linux, mac and windows FAILED: CMakeFiles/clang_rt.builtins-aarch64.dir/cpu_model.c.o

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added a comment. Does anyone have any more objections? I'm going to merge it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127812/new/ https://reviews.llvm.org/D127812 ___ cfe-commits mailing

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

danielkiss accepted this revision. danielkiss added a comment. This revision is now accepted and ready to land. just small comment, thanks @ilinpv LGTM, just let others to chime in. Comment at: clang/lib/Basic/Targets/AArch64.cpp:652 + // AARCH64_ARCH_EXT_NAME feature with

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added inline comments. Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:115 +AARCH64_ARCH_EXT_NAME("rdm", AArch64::AEK_RDM, "+rdm", "-rdm", \ +RDM, "+rdm,+fp-armv8,+neon,+jsconv,+complxnum",

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

dmgreen added inline comments. Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:115 +AARCH64_ARCH_EXT_NAME("rdm", AArch64::AEK_RDM, "+rdm", "-rdm", \ +RDM, "+rdm,+fp-armv8,+neon,+jsconv,+complxnum",

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv added inline comments. Comment at: llvm/include/llvm/Support/AArch64TargetParser.def:115 +AARCH64_ARCH_EXT_NAME("rdm", AArch64::AEK_RDM, "+rdm", "-rdm", \ +RDM, "+rdm,+fp-armv8,+neon,+jsconv,+complxnum",

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

dmgreen added a comment. I can only comment on the target features part of the patch - I've been hoping it would add something similar and looks very useful in its own right. Comment at: clang/lib/Basic/Targets/AArch64.cpp:63 + switch (ArchKind) { + case

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

ilinpv marked an inline comment as done. ilinpv added inline comments. Comment at: compiler-rt/lib/builtins/cpu_model.c:1311 + // CPU features already initialized. + if (__aarch64_cpu_features.features) +return; danielkiss wrote: > I'd add a init value for

[PATCH] D127812: [AArch64] FMV support and necessary target features dependencies.

danielkiss added inline comments. Comment at: clang/lib/Driver/ToolChains/Clang.cpp:7305 + if (Args.hasArg(options::OPT_mno_fmv)) { +CmdArgs.push_back("-target-feature"); maybe we need to add a check here for compiler-rt as rt-lib because today libgcc is