[Openmp-commits] [PATCH] D99656: [AMDGPU][OpenMP] Enable Libomptarget runtime tests

[Jon Chesterfield via Phabricator via Openmp-commits]

JonChesterfield added a comment.

I guess there's no XFAIL equivalent here? In that case we should probably leave off the RUN line for the tests that can't work, as they'll otherwise break the build once an amdgpu CI machine goes live



================
Comment at: openmp/libomptarget/CMakeLists.txt:40
+option(LIBOMPTARGET_ENABLE_EXPERIMENTAL_AMDGCN_TARGET "Enable building plugin and device runtime library for amdgpu" Off)
+option(LIBOMPTARGET_AMDGCN_TEST_TARGET "AMDGPU target to be used in -march=gfx906 when running runtime tests" "gfx908")
+
----------------
I don't understand this string. Maybe in '-march=$LIBOMPTARGET_AMDGCN_TEST_TARGET'? Seems bad to be hard coding a gfx908 here, presumably it should be whatever is in the system running the tests


================
Comment at: openmp/libomptarget/deviceRTLs/CMakeLists.txt:12
 ##===----------------------------------------------------------------------===##
-
+if(LIBOMPTARGET_ENABLE_EXPERIMENTAL_AMDGCN_TARGET)
+    add_subdirectory(amdgcn)
----------------
would rather enable this unconditionally, which works today except for people who build with the amdgpu target disabled. See D98746


================
Comment at: openmp/libomptarget/deviceRTLs/amdgcn/CMakeLists.txt:94
 # create libraries
-set(mcpus gfx700 gfx701 gfx801 gfx803 gfx900 gfx906)
+set(mcpus gfx700 gfx701 gfx801 gfx803 gfx900 gfx906 gfx908)
 if (DEFINED LIBOMPTARGET_AMDGCN_GFXLIST)
----------------
This we can patch separately. Maybe also drop the gfx7? I don't think it's being tested, and if it's broken it would be friendlier to not build it by default.


================
Comment at: openmp/libomptarget/plugins/CMakeLists.txt:80
 
+if(LIBOMPTARGET_ENABLE_EXPERIMENTAL_AMDGCN_TARGET)
+    add_subdirectory(amdgpu)
----------------
This seems to be a regression, why not continue building the plugin unconditionally?


================
Comment at: openmp/libomptarget/test/lit.site.cfg.in:19
 
+# amdgcn target requires -march option to be passed explicitly
+config.libomptarget_amdgcn_test_target = "@LIBOMPTARGET_AMDGCN_TEST_TARGET@"
----------------
this seems very inconvenient for the tests, I think we should handle missing march by looking at the system we're running the tests on


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99656/new/

https://reviews.llvm.org/D99656