[PATCH] D60568: [OpenMP] Add support for registering requires directives with the runtime

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Other tests in presence of the requires directive with the clause?



================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:8975
+        CGM.Error(Clause->getBeginLoc(),
+                  "Target region emitted before requires directive.");
+      HasRequiresUnifiedSharedMemory = true;
----------------
gtbercea wrote:
> ABataev wrote:
> > The message speaks about target region but points to the clause. You need to correct the message.
> "A target region was emitted before this requires directive." ?
Still does not look good, better to add previously emitted target regions in the diagnostic somehow. Also, it would be good to mention the clause in the message.


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:7956
     // Map other list items in the map clause which are not captured variables
-    // but "declare target link" global variables.,
+    // but "declare target link" global variables.
     for (const auto *C : this->CurDir.getClausesOfKind<OMPMapClause>()) {
----------------
Restore the original, this must be changed in a separate patch.


================
Comment at: test/OpenMP/openmp_offload_registration.cpp:40
+// CHECK:     ret void
+// CHECK:     declare void @__tgt_register_requires(i64)
+
----------------
Why do you need this check?


Repository:
  rC Clang

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

https://reviews.llvm.org/D60568