[Openmp-commits] [PATCH] D101030: [OpenMP] Overhaul `declare target` handling

[Johannes Doerfert via Phabricator via Openmp-commits]

jdoerfert marked an inline comment as done.
jdoerfert added a comment.

In D101030#2708277 <https://reviews.llvm.org/D101030#2708277>, @ABataev wrote:

> Could you check that it does not break the tests from https://github.com/clang-ykt/omptests? IIRC, "ref" was introduced to fix a bug with too early optimizations of the declare target variables defined in other modules. Check `t-same-name-definitions` especially, though I'm not sure that exactly this test caused adding of `$ref`s.

I did run that test, works fine. The "$ref"s are still generated, except if you don't have a host version of a variable. Without it, there is no reason to have a ref because you cannot actually address the device version.



================
Comment at: clang/lib/CodeGen/CGOpenMPRuntime.cpp:2999-3000
     if (!hasTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum))
-      initializeTargetRegionEntryInfo(DeviceID, FileID, ParentName, LineNum,
-                                      OffloadingEntriesNum);
     auto &Entry =
----------------
ABataev wrote:
> Why did you decide to drop this?
Because a missing host version should mean we do not create a region entry. Take a look at all the
situations in `clang/test/OpenMP/declare_target_only_one_side_compilation.cpp` for which we are missing a host entry. None of them should create a region entry info.


================
Comment at: clang/lib/Parse/ParseOpenMP.cpp:1736
   SkipUntil(tok::annot_pragma_openmp_end, StopBeforeMatch);
-  ConsumeAnyToken();
-  for (auto &MTLocDecl : DeclareTargetDecls) {
----------------
ABataev wrote:
> Looks like it is still required to consume `tok::annot_pragma_openmp_end`
That's done in the caller now because the clauses are not always parsed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101030