| Message ID | 73773a5061681c502db373df698d2d24b4ad267a.1559562608.git.guillaume.tucker@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [i-g-t,1/4] tests: add libatomic dependency | expand |
On Mon, 2019-06-03 at 12:54 +0100, Guillaume Tucker wrote: > Add dependency to libatomic in order to be able to use the __atomic_* > functions instead of the older __sync_* ones. This is to enable > atomic operations on a wider number of architectures including MIPS. Thanks for your patch! I have a few questions because I don't know well how libatomic works. Do we want to always link against libatomic? For instance LLVM tries to compile a program with atomic before falling back to libatomic: https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/CheckAtomic.cmake Should this dependency be mandatory? > Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com> > --- > meson.build | 1 + > tests/meson.build | 2 +- > 2 files changed, 2 insertions(+), 1 deletion(-) > > diff --git a/meson.build b/meson.build > index 6268c58d3634..4e5bb323fa49 100644 > --- a/meson.build > +++ b/meson.build > @@ -179,6 +179,7 @@ math = cc.find_library('m') > realtime = cc.find_library('rt') > dlsym = cc.find_library('dl') > zlib = cc.find_library('z') > +libatomic = cc.find_library('atomic') > > if cc.has_header('linux/kd.h') > config.set('HAVE_LINUX_KD_H', 1) > diff --git a/tests/meson.build b/tests/meson.build > index 806766e51667..6877ccd59235 100644 > --- a/tests/meson.build > +++ b/tests/meson.build > @@ -233,7 +233,7 @@ i915_progs = [ > 'i915_suspend', > ] > > -test_deps = [ igt_deps ] > +test_deps = [ igt_deps, libatomic ] > > if libdrm_nouveau.found() > test_progs += [
On Mon, Jun 03, 2019 at 12:54:47PM +0100, Guillaume Tucker wrote: > Add dependency to libatomic in order to be able to use the __atomic_* > functions instead of the older __sync_* ones. This is to enable > atomic operations on a wider number of architectures including MIPS. > > Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> This points out that we need some cleanups in tests/meson.build, as it's getting a bit messy with some of the test using igt_desp and others test_deps, but that's out of the sope of this series. Seems like for autotools we already have it: % ag -G Makefile atomic tests/Makefile.am 93:gem_create_LDADD = $(LDADD) -lpthread -latomic 125:sw_sync_LDADD = $(LDADD) -latomic
On 06/06/2019 08:18, Ser, Simon wrote: > On Mon, 2019-06-03 at 12:54 +0100, Guillaume Tucker wrote: >> Add dependency to libatomic in order to be able to use the __atomic_* >> functions instead of the older __sync_* ones. This is to enable >> atomic operations on a wider number of architectures including MIPS. > > Thanks for your patch! I have a few questions because I don't know well > how libatomic works. Thanks for the review! > Do we want to always link against libatomic? For instance LLVM tries to > compile a program with atomic before falling back to libatomic: > https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/CheckAtomic.cmake > > Should this dependency be mandatory? I've had a look around, the short answer is we should make this dependency optional. I'm sending a v2 of this series which addresses this issue. From what I understand, linking against libatomic is actually only needed when some atomic operations aren't supported natively by a CPU architecture. For example, this is the case with 64-bit atomics on 32-bit MIPS. If the CPU can't do an atomic operation and there's no libatomic available, then it won't build. So I've made a test in meson.build to check whether linking against libatomic is required or not, to drop the dependency when it isn't. I've verified that the test passes on x86 (i.e. no dependency) and fails on 32-bit MIPS (i.e. with dependency). Guillaume >> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com> >> --- >> meson.build | 1 + >> tests/meson.build | 2 +- >> 2 files changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/meson.build b/meson.build >> index 6268c58d3634..4e5bb323fa49 100644 >> --- a/meson.build >> +++ b/meson.build >> @@ -179,6 +179,7 @@ math = cc.find_library('m') >> realtime = cc.find_library('rt') >> dlsym = cc.find_library('dl') >> zlib = cc.find_library('z') >> +libatomic = cc.find_library('atomic') >> >> if cc.has_header('linux/kd.h') >> config.set('HAVE_LINUX_KD_H', 1) >> diff --git a/tests/meson.build b/tests/meson.build >> index 806766e51667..6877ccd59235 100644 >> --- a/tests/meson.build >> +++ b/tests/meson.build >> @@ -233,7 +233,7 @@ i915_progs = [ >> 'i915_suspend', >> ] >> >> -test_deps = [ igt_deps ] >> +test_deps = [ igt_deps, libatomic ] >> >> if libdrm_nouveau.found() >> test_progs += [
On Thu, 2019-06-13 at 13:55 +0100, Guillaume Tucker wrote: > On 06/06/2019 08:18, Ser, Simon wrote: > > On Mon, 2019-06-03 at 12:54 +0100, Guillaume Tucker wrote: > > > Add dependency to libatomic in order to be able to use the __atomic_* > > > functions instead of the older __sync_* ones. This is to enable > > > atomic operations on a wider number of architectures including MIPS. > > > > Thanks for your patch! I have a few questions because I don't know well > > how libatomic works. > > Thanks for the review! > > > Do we want to always link against libatomic? For instance LLVM tries to > > compile a program with atomic before falling back to libatomic: > > https://github.com/llvm-mirror/llvm/blob/master/cmake/modules/CheckAtomic.cmake > > > > Should this dependency be mandatory? > > I've had a look around, the short answer is we should make this > dependency optional. I'm sending a v2 of this series which > addresses this issue. > > From what I understand, linking against libatomic is actually > only needed when some atomic operations aren't supported natively > by a CPU architecture. For example, this is the case with 64-bit > atomics on 32-bit MIPS. If the CPU can't do an atomic operation > and there's no libatomic available, then it won't build. So I've > made a test in meson.build to check whether linking against > libatomic is required or not, to drop the dependency when it > isn't. I've verified that the test passes on x86 (i.e. no > dependency) and fails on 32-bit MIPS (i.e. with dependency). Nice! Looks like a good idea to me. > Guillaume > > > > > Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com> > > > --- > > > meson.build | 1 + > > > tests/meson.build | 2 +- > > > 2 files changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/meson.build b/meson.build > > > index 6268c58d3634..4e5bb323fa49 100644 > > > --- a/meson.build > > > +++ b/meson.build > > > @@ -179,6 +179,7 @@ math = cc.find_library('m') > > > realtime = cc.find_library('rt') > > > dlsym = cc.find_library('dl') > > > zlib = cc.find_library('z') > > > +libatomic = cc.find_library('atomic') > > > > > > if cc.has_header('linux/kd.h') > > > config.set('HAVE_LINUX_KD_H', 1) > > > diff --git a/tests/meson.build b/tests/meson.build > > > index 806766e51667..6877ccd59235 100644 > > > --- a/tests/meson.build > > > +++ b/tests/meson.build > > > @@ -233,7 +233,7 @@ i915_progs = [ > > > 'i915_suspend', > > > ] > > > > > > -test_deps = [ igt_deps ] > > > +test_deps = [ igt_deps, libatomic ] > > > > > > if libdrm_nouveau.found() > > > test_progs += [
diff --git a/meson.build b/meson.build index 6268c58d3634..4e5bb323fa49 100644 --- a/meson.build +++ b/meson.build @@ -179,6 +179,7 @@ math = cc.find_library('m') realtime = cc.find_library('rt') dlsym = cc.find_library('dl') zlib = cc.find_library('z') +libatomic = cc.find_library('atomic') if cc.has_header('linux/kd.h') config.set('HAVE_LINUX_KD_H', 1) diff --git a/tests/meson.build b/tests/meson.build index 806766e51667..6877ccd59235 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -233,7 +233,7 @@ i915_progs = [ 'i915_suspend', ] -test_deps = [ igt_deps ] +test_deps = [ igt_deps, libatomic ] if libdrm_nouveau.found() test_progs += [
Add dependency to libatomic in order to be able to use the __atomic_* functions instead of the older __sync_* ones. This is to enable atomic operations on a wider number of architectures including MIPS. Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com> --- meson.build | 1 + tests/meson.build | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-)