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

[Pushpinder Singh via Phabricator via Openmp-commits]

pdhaliwal added inline comments.


================
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")
+
----------------
JonChesterfield wrote:
> 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
Its just a default cmake option value. Since there is no automatic march detection, I thought of having it configurable from command line. I will need dig around for detecting system gpu.


================
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)
----------------
JonChesterfield wrote:
> 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.
I will create a separate patch for it.


================
Comment at: openmp/libomptarget/plugins/CMakeLists.txt:80
 
+if(LIBOMPTARGET_ENABLE_EXPERIMENTAL_AMDGCN_TARGET)
+    add_subdirectory(amdgpu)
----------------
JonChesterfield wrote:
> This seems to be a regression, why not continue building the plugin unconditionally?
This was causing issues when plugin was always getting built but deviceRTL not. When plugin is built, the runtime tests will be executed which are going to fail.


================
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@"
----------------
JonChesterfield wrote:
> 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
Agreed. This seems like a problem which could be solved by Driver/ToolChain. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99656