[PATCH] D144634: [Clang][OpenMP] Support for Code Generation of loop bind clause

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D144634#4272857 <https://reviews.llvm.org/D144634#4272857>, @koops wrote:

> 1. Adding semantic test clang/test/OpenMP/loop_bind_messages.cpp.
> 2. Changes suggested by Alexey.
> 3. >Why need to drop bind clause here? The new Directives to which loop directive is being mapped to, do not contain the bind clause. (a) omp loop bind(parallel)  ----> omp for (b) omp loop bind(teams)  ----->  omp distribute (c) omp loop bind(thread)  ------> omp simd

But why need to drop it? It makes processing more complex. The bind clause itself should not be a problem, no?



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:638
   }
+  void setCurrentDirective(OpenMPDirectiveKind newDK) {
+    SharingMapTy *Top = (SharingMapTy *)getTopOfStackOrNull();
----------------
NewDK


================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:640
+    SharingMapTy *Top = (SharingMapTy *)getTopOfStackOrNull();
+    assert(Top != NULL);
+    Top->Directive = newDK;
----------------
Add message to the assert.


================
Comment at: clang/test/OpenMP/loop_bind_codegen.cpp:2-3
+// expected-no-diagnostics
+#ifndef HEADER
+#define HEADER
+// RUN: %clang_cc1 -DCK1 -verify -fopenmp -x c++ %s -emit-llvm -o - | FileCheck %s
----------------
In other tests, this is used for testing with precompiled headers. You don't have this in your test runs currently.


================
Comment at: clang/test/OpenMP/loop_bind_codegen.cpp:8-13
+/*
+#include <stdio.h>
+#include <assert.h>
+#include <pthread.h>
+#include <omp.h>
+*/
----------------
Remove this commented code too


================
Comment at: clang/test/OpenMP/loop_bind_messages.cpp:1-2
+#ifndef HEADER
+#define HEADER
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fopenmp -fopenmp-version=50 -verify %s
----------------
1. No need for this in semantic test
2, More tests required, nesting of regions, more tests for incompatible clauses.


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

https://reviews.llvm.org/D144634