Maxim and I had a chat, I think this patch means to say, "for now clang will use non optimised code, but deeper analysis is needed to optimise with clang"
On 18 May 2016 at 14:27, Maxim Uvarov <[email protected]> wrote: > On 05/18/16 19:48, Mike Holmes wrote: > >> >> >> On 18 May 2016 at 11:56, Maxim Uvarov <[email protected] <mailto: >> [email protected]>> wrote: >> >> On 05/18/16 18:52, Mike Holmes wrote: >> >> >> >> On 18 May 2016 at 11:15, Maxim Uvarov <[email protected] >> <mailto:[email protected]> >> <mailto:[email protected] >> <mailto:[email protected]>>> wrote: >> >> Fix compilation error for clang with disabling 128 bit >> optimization. >> In function `_odp_atomic_u128_xchg_mm': >> undefined reference to `__atomic_exchange' >> >> Signed-off-by: Maxim Uvarov <[email protected] >> <mailto:[email protected]> >> <mailto:[email protected] >> <mailto:[email protected]>>> >> --- >> I need some quick way to make clang build happy >> >> >> Why not revert whatever introduced the issue ? >> >> . Clean patch can go later. >> >> When is "later" defined to be ? >> >> >> Why dont we just wait for the correct fix ? >> >> to make -m32 work now. >> >> >> why now, why do we need a fix so urgently that we dont fix it properly. >> >> > There is no big urgent and patch can wait usual 24 hours. I might be clear > describing > problem which patch fixes to me understandable for people who did not look > into timer test. > > I think that 2 Olas patches fixes issue with 128 bit optimization with > gcc, but introduce some > other things which we did not capture on review process: > > 1) clangs (at least my Ubuntu clang version 3.4-1ubuntu3 > (tags/RELEASE_34/final) (based on LLVM 3.4) > does not link with gcc build in __atomic_exchange. That means clang build > should use generic not optimized > for 128 bit version. > > 2) Build for odp-linux has to be reproducible. And at the same time run on > any similar arch. > That means that all such optimizations should be under ./configure options > (for timer it's #define ODP_ATOMIC_U128). > Only in that case we can be sure that x86 generic build (which will be in > ubuntu, debian, redhat and etc) will run on > all machines, even which do not support intrisics. > > 3) configure compiller detection things has to be in: > platform/linux-generic/m4/configure.m4 > > > Current patch fixes (1). But because (2) and (3) is only dance around > configure.ac options and no functional change > expected, I think current patch might be the best approach for now. ./ > configure.ac and reproducible build as well as > move timer arch code separate file is subject for other patch and it's not > related to timer internal implementation. > > What is your opinion? > > Maxim. > > > >> Maxim. >> >> >> >> Maxim. >> >> platform/linux-generic/include/odp_atomic_internal.h | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git >> a/platform/linux-generic/include/odp_atomic_internal.h >> b/platform/linux-generic/include/odp_atomic_internal.h >> index 3c5606c..31c8059 100644 >> --- a/platform/linux-generic/include/odp_atomic_internal.h >> +++ b/platform/linux-generic/include/odp_atomic_internal.h >> @@ -590,7 +590,9 @@ static inline void >> _odp_atomic_flag_clear(_odp_atomic_flag_t *flag) >> /* Check if target and compiler supports 128-bit scalars and >> corresponding >> * exchange and CAS operations */ >> /* GCC on x86-64 needs -mcx16 compiler option */ >> -#if defined __SIZEOF_INT128__ && defined >> __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 >> +#if defined(__SIZEOF_INT128__) && \ >> + defined(__GCC_HAVE_SYNC_COMPARE_AND_SWAP_16) && \ >> + !defined(__clang__) >> >> /** Preprocessor symbol that indicates support for >> 128-bit atomics */ >> #define ODP_ATOMIC_U128 >> -- >> 2.7.1.250.gff4ea60 >> >> _______________________________________________ >> lng-odp mailing list >> [email protected] <mailto:[email protected]> >> <mailto:[email protected] >> <mailto:[email protected]>> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> -- Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software >> for ARM SoCs >> "Work should be fun and collaborative, the rest follows" >> >> >> >> >> >> -- >> Mike Holmes >> Technical Manager - Linaro Networking Group >> Linaro.org <http://www.linaro.org/>***│ *Open source software for ARM >> SoCs >> "Work should be fun and collaborative, the rest follows" >> >> > -- Mike Holmes Technical Manager - Linaro Networking Group Linaro.org <http://www.linaro.org/> *│ *Open source software for ARM SoCs "Work should be fun and collaborative, the rest follows" _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
