On 18 May 2016 at 23:24, Ola Liljedahl <[email protected]> wrote:
> Maxim, > > I configured and built using clang and -m32 and it worked for me. clang > chooses the lock-based path. > This means __SIZEOF_INT128__ and __GCC_HAVE_SYNC_COMPARE_AND_SWAP_16 were > not defined so the ODP atomic support for 128-bit variables was not > enabled. The timer code fell back to the lock-based path. > > I used clang 3.6.2-1 running on Ubuntu 15.10. > > mine is ubuntu 14.04 clang version 3.4 which looks like buggy. > I think your clang 3.4 is buggy, it claims to support -mcx16 also for -m32 > targets and then defines those preprocessor symbols above which are tested > by odp_atomic_internal.h. But when generating code, the compiler backend > realises that there is no suitable instruction (e.g. cmpxchg16) for > i386/686 targets so generates calls to external functions (e.g. > __atomic_exchange) instead. But those functions either do not exist or are > somehow not included in the linking (I would expect them to be located in > clang's equivalent to libgcc.a). There are some bug reports on missing > support for 128-bit atomics in clang and instead you get these calls to > non-existing functions. > > Add an additional check to odp_atomic_internal.h that clang version must > be >= 3.6 (don't know about 3.5) for the ODP support for 128 bit atomics to > be enabled. > > -- Ola > Ok, thanks for explanation. I will send v2 of my patch with clang version. Maxim. > > > On 18 May 2016 at 20:53, Maxim Uvarov <[email protected]> wrote: > >> On 05/18/16 21:45, Mike Holmes wrote: >> >>> 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" >>> >>> yes, looks like comment under "---" was confusing. This patch is not a >> hack it's only routes clang generated code to lock path instead of lock >> free path with 128 bit instructions. >> >> Maxim. >> >> On 18 May 2016 at 14:27, Maxim Uvarov <[email protected] <mailto: >>> [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]> >>> <mailto:[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]>> >>> <mailto:[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]>> >>> <mailto:[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 <http://configure.ac> options and no >>> functional change >>> expected, I think current patch might be the best approach for >>> now. ./configure.ac <http://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]>> >>> <mailto:[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 >> > > _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
