[Openmp-commits] [PATCH] D14253: [OpenMP] Initial implementation of OpenMP offloading library - libomptarget plugins.

[George Rokos via Phabricator via Openmp-commits]

grokos added inline comments.


================
Comment at: libomptarget/plugins/cuda/CMakeLists.txt:22
+    
+      if(CMAKE_BUILD_TYPE MATCHES Debug)
+          add_definitions(-DCUDA_ERROR_REPORT)
----------------
Hahnfeld wrote:
> grokos wrote:
> > Hahnfeld wrote:
> > > `LIBOMPTARGET_CMAKE_BUILD_TYPE` to match against lowercase?
> > Done.
> I think this should then be lowercase `debug` to exactly match what's done in the parent directory
Good catch! I've fixed it.


================
Comment at: libomptarget/plugins/cuda/src/rtl.cpp:25-31
+#ifndef TARGET_NAME
+#define TARGET_NAME Generic - 64bit
+#endif
+
+#define GETNAME2(name) #name
+#define GETNAME(name) GETNAME2(name)
+#define DP(...) DEBUGP("Target " GETNAME(TARGET_NAME) " RTL", __VA_ARGS__)
----------------
Hahnfeld wrote:
> grokos wrote:
> > Hahnfeld wrote:
> > > Can `TARGET_NAME` be hardcoded to `CUDA`?
> > TARGET_NAME is set via cmake. We could hadcode it, but the current scheme gives the flexibility to set the name at will (e.g. add version info, etc.) without tampering with the source code.
> All right. But please change the fallback to say `CUDA`
Done.


================
Comment at: libomptarget/plugins/generic-elf-64bit/src/rtl.cpp:129-170
+  // Is the library version incompatible with the header file?
+  if (elf_version(EV_CURRENT) == EV_NONE) {
+    DP("Incompatible ELF library!\n");
+    return 0;
+  }
+
+  char *img_begin = (char *)image->ImageStart;
----------------
Hahnfeld wrote:
> grokos wrote:
> > Hahnfeld wrote:
> > > This looks the same as in the CUDA plugin. Can all this be refactored into a function?
> > True, it's the very same code but it just happens to be so. Another plugin for a different architecture may implement this interface function in a different way. Plugins are meant to be developed independently from each other and their code should not reply upon the existence of some common functionality / refactored code.
> Maybe we can put a function like `elf_check_machine(__tgt_device_image *image, uint16_t target)` into a file `plugins/common/elf.c` and compile it into both plugins? The problem with duplicated code is that you always forget to sync them if you change something.
OK, I created a new `elf_common.c` under `plugins/common` defining this function. The file is inlined into both plugins (to inherit the definition of macro `DP` for each plugin).


https://reviews.llvm.org/D14253