[Openmp-commits] [PATCH] D65001: [OpenMP][libomptarget] Add support for unified memory for regular maps

[Gheorghe-Teodor Bercea via Phabricator via Openmp-commits]

gtbercea marked an inline comment as done.
gtbercea added inline comments.


================
Comment at: openmp/trunk/libomptarget/test/unified_shared_memory/shared_update.c:28-30
+  // Manual registration of requires flags for Clang versions
+  // that do not support requires.
+  __tgt_register_requires(8);
----------------
Hahnfeld wrote:
> gtbercea wrote:
> > Hahnfeld wrote:
> > > Apparently, this does not work: The generated code will call `__tgt_register_lib` first which caches the global `RequiresFlags` in `Device.RTLRequiresFlags`. Because `__tgt_register_requires` has not been called yet, the value is still 0 so the new code won't be executed. Please fix and test on your end that it works with older versions of the compiler!
> > @Hahnfeld this test works correctly for compiler versions which support unified shared memory.
> > 
> > My proposed fix is to remove all the manual calls and restrict the test to new versions of Clang and do that for every test here and in the close modifier patches. None of the close or unified memory pieces of functionality need to be tested with older clang versions because they are not supported on those versions.
> This will lose a lot of test coverage for the runtime library because the tests can only be run with a not-even-released versions of the compiler.
New features are only added to the newest version of the compiler and it will work with that and I think that's where the tests need to pass to make sure that incoming functionality doesn't break existing one.

I don't see any reason to test with older versions of Clang that can't even use these features. Am I missing something? What's the gain?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D65001