[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGeb3e81f43f01: [OpenMP][NFCI] Introduce 
llvm/IR/OpenMPConstants.h (authored by jdoerfert).

Changed prior to commit:
  https://reviews.llvm.org/D69853?vs=232987=233003#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/CMakeLists.txt
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/CMakeLists.txt
  llvm/lib/Frontend/CMakeLists.txt
  llvm/lib/Frontend/LLVMBuild.txt
  llvm/lib/Frontend/OpenMP/CMakeLists.txt
  llvm/lib/Frontend/OpenMP/LLVMBuild.txt
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/LLVMBuild.txt

Index: llvm/lib/Frontend/OpenMP/OMPConstants.cpp
===
--- /dev/null
+++ llvm/lib/Frontend/OpenMP/OMPConstants.cpp
@@ -0,0 +1,34 @@
+//===- OMPConstants.cpp - Helpers related to OpenMP code generation ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+StringRef llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/lib/Frontend/OpenMP/LLVMBuild.txt
===
--- llvm/lib/Frontend/OpenMP/LLVMBuild.txt
+++ llvm/lib/Frontend/OpenMP/LLVMBuild.txt
@@ -1,4 +1,4 @@
-;===- ./lib/LLVMBuild.txt --*- Conf -*--===;
+;===- ./lib/Frontend/OpenMP/LLVMBuild.txt --*- Conf -*--===;
 ;
 ; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 ; See https://llvm.org/LICENSE.txt for license information.
@@ -14,42 +14,8 @@
 ;
 ;======;
 
-[common]
-subdirectories =
- Analysis
- AsmParser
- Bitcode
- Bitstream
- CodeGen
- DebugInfo
- Demangle
- ExecutionEngine
- FuzzMutate
- LineEditor
- Linker
- IR
- IRReader
- LTO
- MC
- MCA
- Object
- BinaryFormat
- ObjectYAML
- Option
- Remarks
- Passes
- ProfileData
- Support
- TableGen
- TextAPI
- Target
- Testing
- ToolDrivers
- Transforms
- WindowsManifest
- XRay
-
 [component_0]
-type = Group
-name = Libraries
-parent = $ROOT
+type = Library
+name = FrontendOpenMP
+parent = Frontend
+required_libraries = Core Support TransformUtils
Index: llvm/lib/Frontend/OpenMP/CMakeLists.txt
===
--- /dev/null
+++ llvm/lib/Frontend/OpenMP/CMakeLists.txt
@@ -0,0 +1,10 @@
+add_llvm_component_library(LLVMFrontendOpenMP
+  OMPConstants.cpp
+
+  ADDITIONAL_HEADER_DIRS
+  ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend
+  ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend/OpenMP
+
+  DEPENDS
+  intrinsics_gen
+  )
Index: llvm/lib/LLVMBuild.txt
===
--- llvm/lib/LLVMBuild.txt
+++ llvm/lib/LLVMBuild.txt
@@ -24,6 +24,7 @@
  DebugInfo
  Demangle
  ExecutionEngine
+ Frontend
  FuzzMutate
  LineEditor
  Linker
Index: llvm/lib/Frontend/LLVMBuild.txt
===
--- llvm/lib/Frontend/LLVMBuild.txt
+++ llvm/lib/Frontend/LLVMBuild.txt
@@ -1,4 +1,4 @@
-;===- 

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[pre-merge checks [bot] via Phabricator via cfe-commits]

merge_guards_bot added a comment.

Build result: pass - 60659 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt 
,
 CMakeCache.txt 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 232987.
jdoerfert added a comment.

Add missing files


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/CMakeLists.txt
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/CMakeLists.txt
  llvm/lib/Frontend/CMakeLists.txt
  llvm/lib/Frontend/LLVMBuild.txt
  llvm/lib/Frontend/OpenMP/CMakeLists.txt
  llvm/lib/Frontend/OpenMP/LLVMBuild.txt
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/LLVMBuild.txt
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -70,7 +70,7 @@
 "LangStandards.cpp",
 "Module.cpp",
 "ObjCRuntime.cpp",
-"OpenMPKinds.cpp",
+"OMPKinds.cpp",
 "OperatorPrecedence.cpp",
 "SanitizerBlacklist.cpp",
 "SanitizerSpecialCaseList.cpp",
Index: llvm/lib/Frontend/OpenMP/OMPConstants.cpp
===
--- /dev/null
+++ llvm/lib/Frontend/OpenMP/OMPConstants.cpp
@@ -0,0 +1,34 @@
+//===- OMPConstants.cpp - Helpers related to OpenMP code generation ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+StringRef llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/lib/Frontend/OpenMP/LLVMBuild.txt
===
--- llvm/lib/Frontend/OpenMP/LLVMBuild.txt
+++ llvm/lib/Frontend/OpenMP/LLVMBuild.txt
@@ -1,4 +1,4 @@
-;===- ./lib/LLVMBuild.txt --*- Conf -*--===;
+;===- ./lib/Frontend/OpenMP/LLVMBuild.txt --*- Conf -*--===;
 ;
 ; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 ; See https://llvm.org/LICENSE.txt for license information.
@@ -14,42 +14,8 @@
 ;
 ;======;
 
-[common]
-subdirectories =
- Analysis
- AsmParser
- Bitcode
- Bitstream
- CodeGen
- DebugInfo
- Demangle
- ExecutionEngine
- FuzzMutate
- LineEditor
- Linker
- IR
- IRReader
- LTO
- MC
- MCA
- Object
- BinaryFormat
- ObjectYAML
- Option
- Remarks
- Passes
- ProfileData
- Support
- TableGen
- TextAPI
- Target
- Testing
- ToolDrivers
- Transforms
- WindowsManifest
- XRay
-
 [component_0]
-type = Group
-name = Libraries
-parent = $ROOT
+type = Library
+name = FrontendOpenMP
+parent = Frontend
+required_libraries = Core Support TransformUtils
Index: llvm/lib/Frontend/OpenMP/CMakeLists.txt
===
--- /dev/null
+++ llvm/lib/Frontend/OpenMP/CMakeLists.txt
@@ -0,0 +1,10 @@
+add_llvm_component_library(LLVMFrontendOpenMP
+  OMPConstants.cpp
+
+  ADDITIONAL_HEADER_DIRS
+  ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend
+  ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend/OpenMP
+
+  DEPENDS
+  intrinsics_gen
+  )
Index: llvm/lib/LLVMBuild.txt
===
--- llvm/lib/LLVMBuild.txt
+++ 

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 232984.
jdoerfert added a comment.

Rebase and creation of libFrontendOpenMP


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/CMakeLists.txt
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/Frontend/OpenMP/OMPConstants.h
  llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
  llvm/lib/CMakeLists.txt
  llvm/lib/Frontend/CMakeLists.txt
  llvm/lib/Frontend/LLVMBuild.txt
  llvm/lib/Frontend/OpenMP/OMPConstants.cpp
  llvm/lib/LLVMBuild.txt
  llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn

Index: llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
===
--- llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
+++ llvm/utils/gn/secondary/clang/lib/Basic/BUILD.gn
@@ -70,7 +70,7 @@
 "LangStandards.cpp",
 "Module.cpp",
 "ObjCRuntime.cpp",
-"OpenMPKinds.cpp",
+"OMPKinds.cpp",
 "OperatorPrecedence.cpp",
 "SanitizerBlacklist.cpp",
 "SanitizerSpecialCaseList.cpp",
Index: llvm/lib/Frontend/OpenMP/OMPConstants.cpp
===
--- /dev/null
+++ llvm/lib/Frontend/OpenMP/OMPConstants.cpp
@@ -0,0 +1,34 @@
+//===- OMPConstants.cpp - Helpers related to OpenMP code generation ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+StringRef llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/Frontend/OpenMP/OMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/lib/LLVMBuild.txt
===
--- llvm/lib/LLVMBuild.txt
+++ llvm/lib/LLVMBuild.txt
@@ -24,6 +24,7 @@
  DebugInfo
  Demangle
  ExecutionEngine
+ Frontend
  FuzzMutate
  LineEditor
  Linker
Index: llvm/lib/Frontend/LLVMBuild.txt
===
--- llvm/lib/Frontend/LLVMBuild.txt
+++ llvm/lib/Frontend/LLVMBuild.txt
@@ -1,4 +1,4 @@
-;===- ./lib/LLVMBuild.txt --*- Conf -*--===;
+;===- ./lib/Frontend/LLVMBuild.txt -*- Conf -*--===;
 ;
 ; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 ; See https://llvm.org/LICENSE.txt for license information.
@@ -15,41 +15,9 @@
 ;======;
 
 [common]
-subdirectories =
- Analysis
- AsmParser
- Bitcode
- Bitstream
- CodeGen
- DebugInfo
- Demangle
- ExecutionEngine
- FuzzMutate
- LineEditor
- Linker
- IR
- IRReader
- LTO
- MC
- MCA
- Object
- BinaryFormat
- ObjectYAML
- Option
- Remarks
- Passes
- ProfileData
- Support
- TableGen
- TextAPI
- Target
- Testing
- ToolDrivers
- Transforms
- WindowsManifest
- XRay
+subdirectories = OpenMP
 
 [component_0]
 type = Group
-name = Libraries
-parent = $ROOT
+name = Frontend
+parent = Libraries
Index: llvm/lib/Frontend/CMakeLists.txt
===
--- /dev/null
+++ llvm/lib/Frontend/CMakeLists.txt
@@ -0,0 +1 @@
+add_subdirectory(OpenMP)
Index: llvm/lib/CMakeLists.txt
===
--- llvm/lib/CMakeLists.txt
+++ llvm/lib/CMakeLists.txt
@@ -8,6 +8,7 @@
 add_subdirectory(BinaryFormat)
 

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 4 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.h:23
 /// OpenMP directives.
-enum OpenMPDirectiveKind {
-#define OPENMP_DIRECTIVE(Name) \
-  OMPD_##Name,
-#define OPENMP_DIRECTIVE_EXT(Name, Str) \
-  OMPD_##Name,
-#include "clang/Basic/OpenMPKinds.def"
-  OMPD_unknown
-};
+using OpenMPDirectiveKind = llvm::omp::Directive;
 

fpetrogalli wrote:
> I am not a fan of `using` in header files in general contexts. It's mostly a 
> stylistic preference. Why not use `llvm::omp::Directive` everywhere? It is 
> not much longer than the substitution...
I did not do that to keep the diff as concise as possible. It is big enough as 
it is (I think) and this would add a lot of noise.
Do I need to change that now or are you OK with this? 



Comment at: clang/lib/Basic/OpenMPKinds.cpp:384
 OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
+  assert(unsigned(DKind) <= unsigned(OMPD_unknown));
   assert(CKind <= OMPC_unknown);

fpetrogalli wrote:
> I really wish we would not have to add all these casts. They make the code 
> ugly. I prefer `enum class` over `enum`, but if it results in this because 
> most of the code uses enums, maybe is worth using just `enum`.
It seems so far most people are fine with the enum class if you now changed 
your mind and want to reopen this discussion, let me know please. 
FWIW, I'd argue we should minimize the use as numbers instead of going back on 
type safety. In the new code we only cast it a single time and there we 
actually want to use it as a number in a runtime call we generate.



Comment at: clang/lib/Serialization/ASTWriter.cpp:6625
 void OMPClauseWriter::VisitOMPClauseWithPreInit(OMPClauseWithPreInit *C) {
-  Record.push_back(C->getCaptureRegion());
+  Record.push_back(uint64_t(C->getCaptureRegion()));
   Record.AddStmt(C->getPreInitStmt());

fpetrogalli wrote:
> hum... is this really necessary? Why not make the type of the elements of 
> `Record` the same as the type returned by `C->getCaptureregion()`?
Because this is generic clang serialization code. It has to lower to a byte 
sequence eventually, enum classes need explicit lowering.



Comment at: llvm/include/llvm/Frontend/OpenMPConstants.h:29-34
+/// Make the enum values available in the llvm::omp namespace. This allows us 
to
+/// write something like OMPD_parallel if we have a `using namespace omp`. At
+/// the same time we do not loose the strong type guarantees of the enum class,
+/// that is we cannot pass an unsigned as Directive without an explicit cast.
+#define OMP_DIRECTIVE(Enum, ...) constexpr auto Enum = omp::Directive::Enum;
+#include "llvm/Frontend/OpenMPKinds.def"

fpetrogalli wrote:
> Is this really necessary? What's wrong with writing 
> `Directive::OMPD_parallel` when `using namespace omp`?
> 
> Also, isn't the information provided by `OMPD` a duplicate of 
> `omp::Directive`? In my mind the acronym expands to **O**pen**MP** 
> **D**irective...
> 
> Isn't the following more concise?
> 
> ```
> omp::Directive::parallel
> omp::Directive::declare_simd
> omp::Directive::whatever
> ```
> 
> IMHO, less is better :)
That doesn't work because some directives are C++ keywords.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Francesco Petrogalli via Phabricator via cfe-commits]

fpetrogalli added inline comments.



Comment at: clang/include/clang/Basic/OpenMPKinds.h:23
 /// OpenMP directives.
-enum OpenMPDirectiveKind {
-#define OPENMP_DIRECTIVE(Name) \
-  OMPD_##Name,
-#define OPENMP_DIRECTIVE_EXT(Name, Str) \
-  OMPD_##Name,
-#include "clang/Basic/OpenMPKinds.def"
-  OMPD_unknown
-};
+using OpenMPDirectiveKind = llvm::omp::Directive;
 

I am not a fan of `using` in header files in general contexts. It's mostly a 
stylistic preference. Why not use `llvm::omp::Directive` everywhere? It is not 
much longer than the substitution...



Comment at: clang/lib/Basic/OpenMPKinds.cpp:384
 OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
+  assert(unsigned(DKind) <= unsigned(OMPD_unknown));
   assert(CKind <= OMPC_unknown);

I really wish we would not have to add all these casts. They make the code 
ugly. I prefer `enum class` over `enum`, but if it results in this because most 
of the code uses enums, maybe is worth using just `enum`.



Comment at: clang/lib/Serialization/ASTWriter.cpp:6625
 void OMPClauseWriter::VisitOMPClauseWithPreInit(OMPClauseWithPreInit *C) {
-  Record.push_back(C->getCaptureRegion());
+  Record.push_back(uint64_t(C->getCaptureRegion()));
   Record.AddStmt(C->getPreInitStmt());

hum... is this really necessary? Why not make the type of the elements of 
`Record` the same as the type returned by `C->getCaptureregion()`?



Comment at: llvm/include/llvm/Frontend/OpenMPConstants.h:29-34
+/// Make the enum values available in the llvm::omp namespace. This allows us 
to
+/// write something like OMPD_parallel if we have a `using namespace omp`. At
+/// the same time we do not loose the strong type guarantees of the enum class,
+/// that is we cannot pass an unsigned as Directive without an explicit cast.
+#define OMP_DIRECTIVE(Enum, ...) constexpr auto Enum = omp::Directive::Enum;
+#include "llvm/Frontend/OpenMPKinds.def"

Is this really necessary? What's wrong with writing `Directive::OMPD_parallel` 
when `using namespace omp`?

Also, isn't the information provided by `OMPD` a duplicate of `omp::Directive`? 
In my mind the acronym expands to **O**pen**MP** **D**irective...

Isn't the following more concise?

```
omp::Directive::parallel
omp::Directive::declare_simd
omp::Directive::whatever
```

IMHO, less is better :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228788.
jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

Create llvm/libFrontend, move code there, make shared library builds work.

llvm/IR/ is an unfortunate location as we cannot use llvm utility functions
without introducing dependences between the libraries that are unwanted.

The new libFrontend should not only house the OpenMP-IR-Builder but other code
parts shared between different frontends later on.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/CMakeLists.txt
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/CMakeLists.txt
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/CMakeLists.txt
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/Frontend/OpenMPConstants.h
  llvm/include/llvm/Frontend/OpenMPKinds.def
  llvm/lib/CMakeLists.txt
  llvm/lib/Frontend/CMakeLists.txt
  llvm/lib/Frontend/LLVMBuild.txt
  llvm/lib/Frontend/OpenMPConstants.cpp
  llvm/lib/LLVMBuild.txt

Index: llvm/lib/Frontend/OpenMPConstants.cpp
===
--- /dev/null
+++ llvm/lib/Frontend/OpenMPConstants.cpp
@@ -0,0 +1,34 @@
+//===- OpenMPConstants.cpp - Helpers related to OpenMP code generation ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/Frontend/OpenMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/Frontend/OpenMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+StringRef llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/Frontend/OpenMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/lib/LLVMBuild.txt
===
--- llvm/lib/LLVMBuild.txt
+++ llvm/lib/LLVMBuild.txt
@@ -24,6 +24,7 @@
  DebugInfo
  Demangle
  ExecutionEngine
+ Frontend
  FuzzMutate
  LineEditor
  Linker
Index: llvm/lib/Frontend/LLVMBuild.txt
===
--- llvm/lib/Frontend/LLVMBuild.txt
+++ llvm/lib/Frontend/LLVMBuild.txt
@@ -1,4 +1,4 @@
-;===- ./lib/LLVMBuild.txt --*- Conf -*--===;
+;===- ./lib/Frontend/LLVMBuild.txt -*- Conf -*--===;
 ;
 ; Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 ; See https://llvm.org/LICENSE.txt for license information.
@@ -14,42 +14,8 @@
 ;
 ;======;
 
-[common]
-subdirectories =
- Analysis
- AsmParser
- Bitcode
- Bitstream
- CodeGen
- DebugInfo
- Demangle
- ExecutionEngine
- FuzzMutate
- LineEditor
- Linker
- IR
- IRReader
- LTO
- MC
- MCA
- Object
- BinaryFormat
- ObjectYAML
- Option
- Remarks
- Passes
- ProfileData
- Support
- TableGen
- TextAPI
- Target
- Testing
- ToolDrivers
- Transforms
- WindowsManifest
- XRay
-
 [component_0]
-type = Group
-name = Libraries
-parent = $ROOT
+type = Library
+name = Frontend
+parent = Libraries
+required_libraries = Core Support TransformUtils
Index: llvm/lib/Frontend/CMakeLists.txt
===
--- /dev/null
+++ llvm/lib/Frontend/CMakeLists.txt
@@ -0,0 +1,9 @@
+add_llvm_library(LLVMFrontend
+  OpenMPConstants.cpp
+
+  ADDITIONAL_HEADER_DIRS
+  ${LLVM_MAIN_INCLUDE_DIR}/llvm/Frontend
+
+  DEPENDS
+  intrinsics_gen
+  )
Index: llvm/lib/CMakeLists.txt
===
--- llvm/lib/CMakeLists.txt
+++ llvm/lib/CMakeLists.txt
@@ -8,6 +8,7 @@
 add_subdirectory(BinaryFormat)
 

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev accepted this revision.
ABataev added a comment.
This revision is now accepted and ready to land.

LG with a nit




Comment at: llvm/include/llvm/IR/OpenMPConstants.h:1
+//===- IR/OpenMPConstants.h - OpenMP related constants & helper - C++ -*-===//
+//

Just `OpenMPConstants.h`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

In D69853#1737426 , @ABataev wrote:

> In D69853#1737319 , @jdoerfert wrote:
>
> > In D69853#1737264 , @ABataev wrote:
> >
> > > In D69853#1737218 , @jdoerfert 
> > > wrote:
> > >
> > > > Are there  blocking issues on this one or can we go ahead and adjust 
> > > > minor details as we go?
> > >
> > >
> > > I still think it would be good to separate patches into the LLVM part and 
> > > into the clang part and commit them separately? E.g. flang people may try 
> > > to look for the commits for constant in LLVM, but not for the clang 
> > > changes. It will be much easier for them to skip the changes in clang.
> >
> >
> > But they might also want to see how to interact with the constants so 
> > having the clang parts in there is good. I do not see a clear benefit, 
> > e.g., Flang ppl might or might not prefer a separate patch, but I see a 
> > clear drawback (testing wise) so I still don't believe splitting the right 
> > thing.
>
>
> Testing is not a problem for NFC LLVM part of the patch, I think. Plus, this 
> whole patch is NFC.


That is not a reason not to test it. And again, there are arguments for a 
combined patch even beyond testing.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:4008
+struct Kind2Unsigned {
+  using argument_type = OpenMPDirectiveKind;
+  unsigned operator()(argument_type DK) { return unsigned(DK); }

ABataev wrote:
> `ArgumentType`
it has to be `argument_type`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69853#1737319 , @jdoerfert wrote:

> In D69853#1737264 , @ABataev wrote:
>
> > In D69853#1737218 , @jdoerfert 
> > wrote:
> >
> > > Are there  blocking issues on this one or can we go ahead and adjust 
> > > minor details as we go?
> >
> >
> > I still think it would be good to separate patches into the LLVM part and 
> > into the clang part and commit them separately? E.g. flang people may try 
> > to look for the commits for constant in LLVM, but not for the clang 
> > changes. It will be much easier for them to skip the changes in clang.
>
>
> But they might also want to see how to interact with the constants so having 
> the clang parts in there is good. I do not see a clear benefit, e.g., Flang 
> ppl might or might not prefer a separate patch, but I see a clear drawback 
> (testing wise) so I still don't believe splitting the right thing.


Testing is not a problem for NFC LLVM part of the patch, I think. Plus, this 
whole patch is NFC.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:4008
+struct Kind2Unsigned {
+  using argument_type = OpenMPDirectiveKind;
+  unsigned operator()(argument_type DK) { return unsigned(DK); }

`ArgumentType`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228275.
jdoerfert marked an inline comment as done.
jdoerfert added a comment.

Use an llvm::IndexedMap instead of std::map


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp

Index: llvm/lib/IR/OpenMPConstants.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPConstants.cpp
@@ -0,0 +1,34 @@
+//===- OpenMPConstants.cpp - Helpers related to OpenMP code generation ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/IR/OpenMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/IR/OpenMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+StringRef llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/lib/IR/CMakeLists.txt
===
--- llvm/lib/IR/CMakeLists.txt
+++ llvm/lib/IR/CMakeLists.txt
@@ -39,6 +39,7 @@
   Metadata.cpp
   Module.cpp
   ModuleSummaryIndex.cpp
+  OpenMPConstants.cpp
   Operator.cpp
   OptBisect.cpp
   Pass.cpp
Index: llvm/include/llvm/IR/OpenMPKinds.def
===
--- /dev/null
+++ llvm/include/llvm/IR/OpenMPKinds.def
@@ -0,0 +1,101 @@
+//===--- OpenMPKinds.def - OpenMP directives, clauses, rt-calls -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file defines the list of supported OpenMP directives, clauses, runtime
+/// calls, and other things that need to be listed in enums.
+///
+//===--===//
+
+/// OpenMP Directives and combined directives
+///
+///{
+
+#ifndef OMP_DIRECTIVE
+#define OMP_DIRECTIVE(Enum, Str)
+#endif
+
+#define __OMP_DIRECTIVE_EXT(Name, Str) OMP_DIRECTIVE(OMPD_##Name, Str)
+#define __OMP_DIRECTIVE(Name) __OMP_DIRECTIVE_EXT(Name, #Name)
+
+__OMP_DIRECTIVE(threadprivate)
+__OMP_DIRECTIVE(parallel)
+__OMP_DIRECTIVE(task)
+__OMP_DIRECTIVE(simd)
+__OMP_DIRECTIVE(for)
+__OMP_DIRECTIVE(sections)
+__OMP_DIRECTIVE(section)
+__OMP_DIRECTIVE(single)
+__OMP_DIRECTIVE(master)
+__OMP_DIRECTIVE(critical)
+__OMP_DIRECTIVE(taskyield)
+__OMP_DIRECTIVE(barrier)
+__OMP_DIRECTIVE(taskwait)
+__OMP_DIRECTIVE(taskgroup)
+__OMP_DIRECTIVE(flush)
+__OMP_DIRECTIVE(ordered)
+__OMP_DIRECTIVE(atomic)
+__OMP_DIRECTIVE(target)
+__OMP_DIRECTIVE(teams)
+__OMP_DIRECTIVE(cancel)
+__OMP_DIRECTIVE(requires)
+__OMP_DIRECTIVE_EXT(target_data, "target data")
+__OMP_DIRECTIVE_EXT(target_enter_data, "target enter data")
+__OMP_DIRECTIVE_EXT(target_exit_data, "target exit data")
+__OMP_DIRECTIVE_EXT(target_parallel, "target parallel")
+__OMP_DIRECTIVE_EXT(target_parallel_for, "target parallel for")
+__OMP_DIRECTIVE_EXT(target_update, "target update")
+__OMP_DIRECTIVE_EXT(parallel_for, "parallel for")
+__OMP_DIRECTIVE_EXT(parallel_for_simd, "parallel for simd")
+__OMP_DIRECTIVE_EXT(parallel_sections, "parallel sections")
+__OMP_DIRECTIVE_EXT(for_simd, "for simd")

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 2 inline comments as done.
jdoerfert added a comment.

In D69853#1737264 , @ABataev wrote:

> In D69853#1737218 , @jdoerfert wrote:
>
> > Are there  blocking issues on this one or can we go ahead and adjust minor 
> > details as we go?
>
>
> I still think it would be good to separate patches into the LLVM part and 
> into the clang part and commit them separately? E.g. flang people may try to 
> look for the commits for constant in LLVM, but not for the clang changes. It 
> will be much easier for them to skip the changes in clang.


But they might also want to see how to interact with the constants so having 
the clang parts in there is good. I do not see a clear benefit, e.g., Flang ppl 
might or might not prefer a separate patch, but I see a clear drawback (testing 
wise) so I still don't believe splitting the right thing.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

ABataev wrote:
> jdoerfert wrote:
> > Meinersbur wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > JonChesterfield wrote:
> > > > > > > > > I wonder if it would be worth wrapping the accesses to 
> > > > > > > > > FoundNameModifiers in functor that does the enum class to 
> > > > > > > > > unsigned conversion. E.g. a class instance that contains the 
> > > > > > > > > small vector and exposes operator[] that takes the enum class.
> > > > > > > > > 
> > > > > > > > > FoundNameModifiers[unsigned(val)] is quite a lot of line 
> > > > > > > > > noise.
> > > > > > > > Why `map`? It can be represented as a simple c-style array 
> > > > > > > > without any problems.
> > > > > > > No, these are not enums that auto-convert to unsigned.
> > > > > > Convert them manually. Replacing the simple hash array with map 
> > > > > > does not look like a good solution.
> > > > > I feel this this is a micro optimization that will make the code less 
> > > > > maintainable and, as Jon noted, "FoundNameModifiers[unsigned(val)] is 
> > > > > quite a lot of line noise.". 
> > > > > Take a look at the first version and let me know if you want to go 
> > > > > back to that one for this part, if so, I can do that.
> > > > You already introduced special wrapper class in the same file, maybe 
> > > > you can reuse for implicit conversions?
> > > We are introducing an `EnumeratedArray` class in 
> > > https://reviews.llvm.org/D68827#change-XphX8PAWFr8V . It should reduce 
> > > the need for casting and `OpenMPDirectiveKindExWrapper`.
> > Arguably, the easiest and cleanest way is to use a `map` to *map directives 
> > to if clauses*.
> > 
> > The wrapper is in a different file but I can write another one for here.
> > 
> > EnumeratedArray could probably be used here, it is not in yet though.
> Then better to use `llvm::IndexedMap` or `llvm::DenseMap`
It will not matter but sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69853#1737218 , @jdoerfert wrote:

> Are there  blocking issues on this one or can we go ahead and adjust minor 
> details as we go?


I still think it would be good to separate patches into the LLVM part and into 
the clang part and commit them separately? E.g. flang people may try to look 
for the commits for constant in LLVM, but not for the clang changes. It will be 
much easier for them to skip the changes in clang.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

jdoerfert wrote:
> Meinersbur wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > jdoerfert wrote:
> > > > > > ABataev wrote:
> > > > > > > JonChesterfield wrote:
> > > > > > > > I wonder if it would be worth wrapping the accesses to 
> > > > > > > > FoundNameModifiers in functor that does the enum class to 
> > > > > > > > unsigned conversion. E.g. a class instance that contains the 
> > > > > > > > small vector and exposes operator[] that takes the enum class.
> > > > > > > > 
> > > > > > > > FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
> > > > > > > Why `map`? It can be represented as a simple c-style array 
> > > > > > > without any problems.
> > > > > > No, these are not enums that auto-convert to unsigned.
> > > > > Convert them manually. Replacing the simple hash array with map does 
> > > > > not look like a good solution.
> > > > I feel this this is a micro optimization that will make the code less 
> > > > maintainable and, as Jon noted, "FoundNameModifiers[unsigned(val)] is 
> > > > quite a lot of line noise.". 
> > > > Take a look at the first version and let me know if you want to go back 
> > > > to that one for this part, if so, I can do that.
> > > You already introduced special wrapper class in the same file, maybe you 
> > > can reuse for implicit conversions?
> > We are introducing an `EnumeratedArray` class in 
> > https://reviews.llvm.org/D68827#change-XphX8PAWFr8V . It should reduce the 
> > need for casting and `OpenMPDirectiveKindExWrapper`.
> Arguably, the easiest and cleanest way is to use a `map` to *map directives 
> to if clauses*.
> 
> The wrapper is in a different file but I can write another one for here.
> 
> EnumeratedArray could probably be used here, it is not in yet though.
Then better to use `llvm::IndexedMap` or `llvm::DenseMap`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

Are there  blocking issues on this one or can we go ahead and adjust minor 
details as we go?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 228126.
jdoerfert marked an inline comment as done.
jdoerfert added a comment.

working version


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp

Index: llvm/lib/IR/OpenMPConstants.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPConstants.cpp
@@ -0,0 +1,34 @@
+//===- OpenMPConstants.cpp - Helpers related to OpenMP code generation ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/IR/OpenMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/IR/OpenMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+StringRef llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/lib/IR/CMakeLists.txt
===
--- llvm/lib/IR/CMakeLists.txt
+++ llvm/lib/IR/CMakeLists.txt
@@ -39,6 +39,7 @@
   Metadata.cpp
   Module.cpp
   ModuleSummaryIndex.cpp
+  OpenMPConstants.cpp
   Operator.cpp
   OptBisect.cpp
   Pass.cpp
Index: llvm/include/llvm/IR/OpenMPKinds.def
===
--- /dev/null
+++ llvm/include/llvm/IR/OpenMPKinds.def
@@ -0,0 +1,101 @@
+//===--- OpenMPKinds.def - OpenMP directives, clauses, rt-calls -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file defines the list of supported OpenMP directives, clauses, runtime
+/// calls, and other things that need to be listed in enums.
+///
+//===--===//
+
+/// OpenMP Directives and combined directives
+///
+///{
+
+#ifndef OMP_DIRECTIVE
+#define OMP_DIRECTIVE(Enum, Str)
+#endif
+
+#define __OMP_DIRECTIVE_EXT(Name, Str) OMP_DIRECTIVE(OMPD_##Name, Str)
+#define __OMP_DIRECTIVE(Name) __OMP_DIRECTIVE_EXT(Name, #Name)
+
+__OMP_DIRECTIVE(threadprivate)
+__OMP_DIRECTIVE(parallel)
+__OMP_DIRECTIVE(task)
+__OMP_DIRECTIVE(simd)
+__OMP_DIRECTIVE(for)
+__OMP_DIRECTIVE(sections)
+__OMP_DIRECTIVE(section)
+__OMP_DIRECTIVE(single)
+__OMP_DIRECTIVE(master)
+__OMP_DIRECTIVE(critical)
+__OMP_DIRECTIVE(taskyield)
+__OMP_DIRECTIVE(barrier)
+__OMP_DIRECTIVE(taskwait)
+__OMP_DIRECTIVE(taskgroup)
+__OMP_DIRECTIVE(flush)
+__OMP_DIRECTIVE(ordered)
+__OMP_DIRECTIVE(atomic)
+__OMP_DIRECTIVE(target)
+__OMP_DIRECTIVE(teams)
+__OMP_DIRECTIVE(cancel)
+__OMP_DIRECTIVE(requires)
+__OMP_DIRECTIVE_EXT(target_data, "target data")
+__OMP_DIRECTIVE_EXT(target_enter_data, "target enter data")
+__OMP_DIRECTIVE_EXT(target_exit_data, "target exit data")
+__OMP_DIRECTIVE_EXT(target_parallel, "target parallel")
+__OMP_DIRECTIVE_EXT(target_parallel_for, "target parallel for")
+__OMP_DIRECTIVE_EXT(target_update, "target update")
+__OMP_DIRECTIVE_EXT(parallel_for, "parallel for")
+__OMP_DIRECTIVE_EXT(parallel_for_simd, "parallel for simd")
+__OMP_DIRECTIVE_EXT(parallel_sections, "parallel sections")
+__OMP_DIRECTIVE_EXT(for_simd, "for simd")
+__OMP_DIRECTIVE_EXT(cancellation_point, "cancellation 

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 5 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

Meinersbur wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > JonChesterfield wrote:
> > > > > > > I wonder if it would be worth wrapping the accesses to 
> > > > > > > FoundNameModifiers in functor that does the enum class to 
> > > > > > > unsigned conversion. E.g. a class instance that contains the 
> > > > > > > small vector and exposes operator[] that takes the enum class.
> > > > > > > 
> > > > > > > FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
> > > > > > Why `map`? It can be represented as a simple c-style array without 
> > > > > > any problems.
> > > > > No, these are not enums that auto-convert to unsigned.
> > > > Convert them manually. Replacing the simple hash array with map does 
> > > > not look like a good solution.
> > > I feel this this is a micro optimization that will make the code less 
> > > maintainable and, as Jon noted, "FoundNameModifiers[unsigned(val)] is 
> > > quite a lot of line noise.". 
> > > Take a look at the first version and let me know if you want to go back 
> > > to that one for this part, if so, I can do that.
> > You already introduced special wrapper class in the same file, maybe you 
> > can reuse for implicit conversions?
> We are introducing an `EnumeratedArray` class in 
> https://reviews.llvm.org/D68827#change-XphX8PAWFr8V . It should reduce the 
> need for casting and `OpenMPDirectiveKindExWrapper`.
Arguably, the easiest and cleanest way is to use a `map` to *map directives to 
if clauses*.

The wrapper is in a different file but I can write another one for here.

EnumeratedArray could probably be used here, it is not in yet though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227985.
jdoerfert added a comment.

More updates


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/CMakeLists.txt
  llvm/lib/IR/OpenMPConstants.cpp

Index: llvm/lib/IR/OpenMPConstants.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPConstants.cpp
@@ -0,0 +1,34 @@
+//===- OpenMPConstants.cpp - Helpers related to OpenMP code generation ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/IR/OpenMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/IR/OpenMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+StringRef llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/lib/IR/CMakeLists.txt
===
--- llvm/lib/IR/CMakeLists.txt
+++ llvm/lib/IR/CMakeLists.txt
@@ -39,6 +39,7 @@
   Metadata.cpp
   Module.cpp
   ModuleSummaryIndex.cpp
+  OpenMPConstants.cpp
   Operator.cpp
   OptBisect.cpp
   Pass.cpp
Index: llvm/include/llvm/IR/OpenMPKinds.def
===
--- /dev/null
+++ llvm/include/llvm/IR/OpenMPKinds.def
@@ -0,0 +1,101 @@
+//===--- OpenMPKinds.def - OpenMP directives, clauses, rt-calls -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file defines the list of supported OpenMP directives, clauses, runtime
+/// calls, and other things that need to be listed in enums.
+///
+//===--===//
+
+/// OpenMP Directives and combined directives
+///
+///{
+
+#ifndef OMP_DIRECTIVE
+#define OMP_DIRECTIVE(Enum, Str)
+#endif
+
+#define __OMP_DIRECTIVE_EXT(Name, Str) OMP_DIRECTIVE(OMPD_##Name, Str)
+#define __OMP_DIRECTIVE(Name) __OMP_DIRECTIVE_EXT(Name, #Name)
+
+__OMP_DIRECTIVE(threadprivate)
+__OMP_DIRECTIVE(parallel)
+__OMP_DIRECTIVE(task)
+__OMP_DIRECTIVE(simd)
+__OMP_DIRECTIVE(for)
+__OMP_DIRECTIVE(sections)
+__OMP_DIRECTIVE(section)
+__OMP_DIRECTIVE(single)
+__OMP_DIRECTIVE(master)
+__OMP_DIRECTIVE(critical)
+__OMP_DIRECTIVE(taskyield)
+__OMP_DIRECTIVE(barrier)
+__OMP_DIRECTIVE(taskwait)
+__OMP_DIRECTIVE(taskgroup)
+__OMP_DIRECTIVE(flush)
+__OMP_DIRECTIVE(ordered)
+__OMP_DIRECTIVE(atomic)
+__OMP_DIRECTIVE(target)
+__OMP_DIRECTIVE(teams)
+__OMP_DIRECTIVE(cancel)
+__OMP_DIRECTIVE(requires)
+__OMP_DIRECTIVE_EXT(target_data, "target data")
+__OMP_DIRECTIVE_EXT(target_enter_data, "target enter data")
+__OMP_DIRECTIVE_EXT(target_exit_data, "target exit data")
+__OMP_DIRECTIVE_EXT(target_parallel, "target parallel")
+__OMP_DIRECTIVE_EXT(target_parallel_for, "target parallel for")
+__OMP_DIRECTIVE_EXT(target_update, "target update")
+__OMP_DIRECTIVE_EXT(parallel_for, "parallel for")
+__OMP_DIRECTIVE_EXT(parallel_for_simd, "parallel for simd")
+__OMP_DIRECTIVE_EXT(parallel_sections, "parallel sections")
+__OMP_DIRECTIVE_EXT(for_simd, "for simd")
+__OMP_DIRECTIVE_EXT(cancellation_point, "cancellation point")
+__OMP_DIRECTIVE_EXT(declare_reduction, 

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > jdoerfert wrote:
> > > > ABataev wrote:
> > > > > JonChesterfield wrote:
> > > > > > I wonder if it would be worth wrapping the accesses to 
> > > > > > FoundNameModifiers in functor that does the enum class to unsigned 
> > > > > > conversion. E.g. a class instance that contains the small vector 
> > > > > > and exposes operator[] that takes the enum class.
> > > > > > 
> > > > > > FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
> > > > > Why `map`? It can be represented as a simple c-style array without 
> > > > > any problems.
> > > > No, these are not enums that auto-convert to unsigned.
> > > Convert them manually. Replacing the simple hash array with map does not 
> > > look like a good solution.
> > I feel this this is a micro optimization that will make the code less 
> > maintainable and, as Jon noted, "FoundNameModifiers[unsigned(val)] is quite 
> > a lot of line noise.". 
> > Take a look at the first version and let me know if you want to go back to 
> > that one for this part, if so, I can do that.
> You already introduced special wrapper class in the same file, maybe you can 
> reuse for implicit conversions?
We are introducing an `EnumeratedArray` class in 
https://reviews.llvm.org/D68827#change-XphX8PAWFr8V . It should reduce the need 
for casting and `OpenMPDirectiveKindExWrapper`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > JonChesterfield wrote:
> > > > > I wonder if it would be worth wrapping the accesses to 
> > > > > FoundNameModifiers in functor that does the enum class to unsigned 
> > > > > conversion. E.g. a class instance that contains the small vector and 
> > > > > exposes operator[] that takes the enum class.
> > > > > 
> > > > > FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
> > > > Why `map`? It can be represented as a simple c-style array without any 
> > > > problems.
> > > No, these are not enums that auto-convert to unsigned.
> > Convert them manually. Replacing the simple hash array with map does not 
> > look like a good solution.
> I feel this this is a micro optimization that will make the code less 
> maintainable and, as Jon noted, "FoundNameModifiers[unsigned(val)] is quite a 
> lot of line noise.". 
> Take a look at the first version and let me know if you want to go back to 
> that one for this part, if so, I can do that.
You already introduced special wrapper class in the same file, maybe you can 
reuse for implicit conversions?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

In D69853#1734760 , @jdoerfert wrote:

> In D69853#1734620 , @ABataev wrote:
>
> > Again, better to split into 2 patches, one for LLVM and one for clang.
>
>
> I split the other patch, but this one cannot be split in a reasonable 
> (=testable) manner. Splitting a patch only to have smaller diffs is not the 
> goal.


We don't need to test it. Just split it into 2 parts, llvm and clang, nothing 
else.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > JonChesterfield wrote:
> > > > I wonder if it would be worth wrapping the accesses to 
> > > > FoundNameModifiers in functor that does the enum class to unsigned 
> > > > conversion. E.g. a class instance that contains the small vector and 
> > > > exposes operator[] that takes the enum class.
> > > > 
> > > > FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
> > > Why `map`? It can be represented as a simple c-style array without any 
> > > problems.
> > No, these are not enums that auto-convert to unsigned.
> Convert them manually. Replacing the simple hash array with map does not look 
> like a good solution.
I feel this this is a micro optimization that will make the code less 
maintainable and, as Jon noted, "FoundNameModifiers[unsigned(val)] is quite a 
lot of line noise.". 
Take a look at the first version and let me know if you want to go back to that 
one for this part, if so, I can do that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D69853#1734620 , @ABataev wrote:

> Again, better to split into 2 patches, one for LLVM and one for clang.


I split the other patch, but this one cannot be split in a reasonable 
(=testable) manner. Splitting a patch only to have smaller diffs is not the 
goal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

jdoerfert wrote:
> ABataev wrote:
> > JonChesterfield wrote:
> > > I wonder if it would be worth wrapping the accesses to FoundNameModifiers 
> > > in functor that does the enum class to unsigned conversion. E.g. a class 
> > > instance that contains the small vector and exposes operator[] that takes 
> > > the enum class.
> > > 
> > > FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
> > Why `map`? It can be represented as a simple c-style array without any 
> > problems.
> No, these are not enums that auto-convert to unsigned.
Convert them manually. Replacing the simple hash array with map does not look 
like a good solution.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

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



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

ABataev wrote:
> JonChesterfield wrote:
> > I wonder if it would be worth wrapping the accesses to FoundNameModifiers 
> > in functor that does the enum class to unsigned conversion. E.g. a class 
> > instance that contains the small vector and exposes operator[] that takes 
> > the enum class.
> > 
> > FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
> Why `map`? It can be represented as a simple c-style array without any 
> problems.
No, these are not enums that auto-convert to unsigned.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added a comment.

Again, better to split into 2 patches, one for LLVM and one for clang.




Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

JonChesterfield wrote:
> I wonder if it would be worth wrapping the accesses to FoundNameModifiers in 
> functor that does the enum class to unsigned conversion. E.g. a class 
> instance that contains the small vector and exposes operator[] that takes the 
> enum class.
> 
> FoundNameModifiers[unsigned(val)] is quite a lot of line noise.
Why `map`? It can be represented as a simple c-style array without any problems.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert updated this revision to Diff 227948.
jdoerfert marked 5 inline comments as done.
jdoerfert added a comment.

Addressed review comments, simplified some changes, moved file, ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/OpenMPConstants.cpp

Index: llvm/lib/IR/OpenMPConstants.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPConstants.cpp
@@ -0,0 +1,34 @@
+//===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/IR/OpenMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/IR/OpenMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+StringRef llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/include/llvm/IR/OpenMPKinds.def
===
--- /dev/null
+++ llvm/include/llvm/IR/OpenMPKinds.def
@@ -0,0 +1,101 @@
+//===--- OpenMPKinds.def - OpenMP directives, clauses, rt-calls -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file defines the list of supported OpenMP directives, clauses, runtime
+/// calls, and other things that need to be listed in enums.
+///
+//===--===//
+
+/// OpenMP Directives and combined directives
+///
+///{
+
+#ifndef OMP_DIRECTIVE
+#define OMP_DIRECTIVE(Enum, Str)
+#endif
+
+#define __OMP_DIRECTIVE_EXT(Name, Str) OMP_DIRECTIVE(OMPD_##Name, Str)
+#define __OMP_DIRECTIVE(Name) __OMP_DIRECTIVE_EXT(Name, #Name)
+
+__OMP_DIRECTIVE(threadprivate)
+__OMP_DIRECTIVE(parallel)
+__OMP_DIRECTIVE(task)
+__OMP_DIRECTIVE(simd)
+__OMP_DIRECTIVE(for)
+__OMP_DIRECTIVE(sections)
+__OMP_DIRECTIVE(section)
+__OMP_DIRECTIVE(single)
+__OMP_DIRECTIVE(master)
+__OMP_DIRECTIVE(critical)
+__OMP_DIRECTIVE(taskyield)
+__OMP_DIRECTIVE(barrier)
+__OMP_DIRECTIVE(taskwait)
+__OMP_DIRECTIVE(taskgroup)
+__OMP_DIRECTIVE(flush)
+__OMP_DIRECTIVE(ordered)
+__OMP_DIRECTIVE(atomic)
+__OMP_DIRECTIVE(target)
+__OMP_DIRECTIVE(teams)
+__OMP_DIRECTIVE(cancel)
+__OMP_DIRECTIVE(requires)
+__OMP_DIRECTIVE_EXT(target_data, "target data")
+__OMP_DIRECTIVE_EXT(target_enter_data, "target enter data")
+__OMP_DIRECTIVE_EXT(target_exit_data, "target exit data")
+__OMP_DIRECTIVE_EXT(target_parallel, "target parallel")
+__OMP_DIRECTIVE_EXT(target_parallel_for, "target parallel for")
+__OMP_DIRECTIVE_EXT(target_update, "target update")
+__OMP_DIRECTIVE_EXT(parallel_for, "parallel for")
+__OMP_DIRECTIVE_EXT(parallel_for_simd, "parallel for simd")
+__OMP_DIRECTIVE_EXT(parallel_sections, "parallel sections")
+__OMP_DIRECTIVE_EXT(for_simd, "for simd")
+__OMP_DIRECTIVE_EXT(cancellation_point, "cancellation point")
+__OMP_DIRECTIVE_EXT(declare_reduction, "declare reduction")
+__OMP_DIRECTIVE_EXT(declare_mapper, "declare mapper")
+__OMP_DIRECTIVE_EXT(declare_simd, "declare simd")
+__OMP_DIRECTIVE(taskloop)
+__OMP_DIRECTIVE_EXT(taskloop_simd, "taskloop simd")
+__OMP_DIRECTIVE(distribute)

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Jon Chesterfield via Phabricator via cfe-commits]

JonChesterfield added inline comments.



Comment at: clang/lib/Sema/SemaOpenMP.cpp:4011
   unsigned NamedModifiersNumber = 0;
-  SmallVector FoundNameModifiers(
-  OMPD_unknown + 1);
+  SmallVector
+  FoundNameModifiers(unsigned(OMPD_unknown) + 1);

I wonder if it would be worth wrapping the accesses to FoundNameModifiers in 
functor that does the enum class to unsigned conversion. E.g. a class instance 
that contains the small vector and exposes operator[] that takes the enum class.

FoundNameModifiers[unsigned(val)] is quite a lot of line noise.



Comment at: llvm/lib/IR/OpenMPIRBuilder.cpp:11
+
+#include "llvm/IR/OpenMPConstants.h"
+

Implemented in OpenMPConstants.cpp instead? Functions look usable independent 
of MPIRBuilder.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added inline comments.



Comment at: clang/lib/Basic/OpenMPKinds.cpp:408
 OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
   assert(CKind <= OMPC_unknown);

ABataev wrote:
> Why assert is removed?
I can add it back, I mean it would look like this now:
  `assert(unsigned(DKind) <= unsigned(OMPD_unknown));`

(I thought that the idea of enum classes is not to need it but we can keep it).



Comment at: clang/lib/Parse/ParseOpenMP.cpp:51-60
+// Helper to unify the enum class OpenMPDirectiveKind with its extension
+// OpenMPDirectiveKindEx.
+struct OpenMPDirectiveKindExWrapper {
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKind DK) : Value(unsigned(DK)) {}
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKindEx DKE)
+  : Value(unsigned(DKE)) {}
+  operator unsigned() const { return Value; }

ABataev wrote:
> Why do we need this?
First, I'm open for suggestions on how to do this in a nicer way :)

Why I did it:
The code below uses two enums as if they were unsigned numbers and one of them 
is now an enum class 
which requires you to do explicit casts. Without this trickery here we would 
need to sprinkle `unsigned()` in the code below, especially the array `F` 
which I did not want to do.




Comment at: llvm/include/llvm/IR/OpenMPConstants.h:40
+/// Return a textual representation of the directive \p D.
+const char *getOpenMPDirectiveName(Directive D);
+

ABataev wrote:
> 1. Better to return StringRef, I think.
> 2. Do we really need these 2 convert functions here? Are we going to use them 
> in LLVM or just in clang?
1. Can do, I just moved them in this patch.
2) We will reuse them in Flang so they should be moved (and it seems natural to 
keep these where the enums are defined.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.



Comment at: clang/lib/Basic/OpenMPKinds.cpp:408
 OpenMPClauseKind CKind) {
-  assert(DKind <= OMPD_unknown);
   assert(CKind <= OMPC_unknown);

Why assert is removed?



Comment at: clang/lib/Parse/ParseOpenMP.cpp:51-60
+// Helper to unify the enum class OpenMPDirectiveKind with its extension
+// OpenMPDirectiveKindEx.
+struct OpenMPDirectiveKindExWrapper {
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKind DK) : Value(unsigned(DK)) {}
+  OpenMPDirectiveKindExWrapper(OpenMPDirectiveKindEx DKE)
+  : Value(unsigned(DKE)) {}
+  operator unsigned() const { return Value; }

Why do we need this?



Comment at: llvm/include/llvm/IR/OpenMPConstants.h:40
+/// Return a textual representation of the directive \p D.
+const char *getOpenMPDirectiveName(Directive D);
+

1. Better to return StringRef, I think.
2. Do we really need these 2 convert functions here? Are we going to use them 
in LLVM or just in clang?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69853



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D69853: [OpenMP][NFCI] Introduce llvm/IR/OpenMPConstants.h

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert created this revision.
jdoerfert added reviewers: kiranchandramohan, ABataev, RaviNarayanaswamy, 
gtbercea, grokos, sdmitriev, JonChesterfield, hfinkel, fghanim.
Herald added a subscriber: jholewinski.
Herald added projects: clang, LLVM.

The new OpenMPConstants.h is a location for all OpenMP related constants
(and helpers) to live.

This patch moves the directives there (the enum OpenMPDirectiveKind) and
rewires Clang to use the new location.

Initially part of D69785 .


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D69853

Files:
  clang/include/clang/AST/OpenMPClause.h
  clang/include/clang/AST/StmtOpenMP.h
  clang/include/clang/Basic/OpenMPKinds.def
  clang/include/clang/Basic/OpenMPKinds.h
  clang/lib/AST/OpenMPClause.cpp
  clang/lib/AST/StmtOpenMP.cpp
  clang/lib/Basic/OpenMPKinds.cpp
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGOpenMPRuntimeNVPTX.cpp
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  clang/lib/CodeGen/CodeGenFunction.h
  clang/lib/Parse/ParseOpenMP.cpp
  clang/lib/Sema/SemaOpenMP.cpp
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/lib/Sema/TreeTransform.h
  clang/lib/Serialization/ASTWriter.cpp
  clang/lib/Serialization/ASTWriterStmt.cpp
  llvm/include/llvm/IR/OpenMPConstants.h
  llvm/include/llvm/IR/OpenMPKinds.def
  llvm/lib/IR/OpenMPIRBuilder.cpp

Index: llvm/lib/IR/OpenMPIRBuilder.cpp
===
--- /dev/null
+++ llvm/lib/IR/OpenMPIRBuilder.cpp
@@ -0,0 +1,34 @@
+//===- OpenMPIRBuilder.cpp - Builder for LLVM-IR for OpenMP directives ===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+//
+//===--===//
+
+#include "llvm/IR/OpenMPConstants.h"
+
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/StringSwitch.h"
+
+using namespace llvm;
+using namespace omp;
+
+Directive llvm::omp::getOpenMPDirectiveKind(StringRef Str) {
+  return llvm::StringSwitch(Str)
+#define OMP_DIRECTIVE(Enum, Str) .Case(Str, Enum)
+#include "llvm/IR/OpenMPKinds.def"
+  .Default(OMPD_unknown);
+}
+
+const char *llvm::omp::getOpenMPDirectiveName(Directive Kind) {
+  switch (Kind) {
+#define OMP_DIRECTIVE(Enum, Str)   \
+  case Enum:   \
+return Str;
+#include "llvm/IR/OpenMPKinds.def"
+  }
+  llvm_unreachable("Invalid OpenMP directive kind");
+}
Index: llvm/include/llvm/IR/OpenMPKinds.def
===
--- /dev/null
+++ llvm/include/llvm/IR/OpenMPKinds.def
@@ -0,0 +1,101 @@
+//===--- OpenMPKinds.def - OpenMP directives, clauses, rt-calls -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===--===//
+/// \file
+///
+/// This file defines the list of supported OpenMP directives, clauses, runtime
+/// calls, and other things that need to be listed in enums.
+///
+//===--===//
+
+/// OpenMP Directives and combined directives
+///
+///{
+
+#ifndef OMP_DIRECTIVE
+#define OMP_DIRECTIVE(Enum, Str)
+#endif
+
+#define __OMP_DIRECTIVE_EXT(Name, Str) OMP_DIRECTIVE(OMPD_##Name, Str)
+#define __OMP_DIRECTIVE(Name) __OMP_DIRECTIVE_EXT(Name, #Name)
+
+__OMP_DIRECTIVE(threadprivate)
+__OMP_DIRECTIVE(parallel)
+__OMP_DIRECTIVE(task)
+__OMP_DIRECTIVE(simd)
+__OMP_DIRECTIVE(for)
+__OMP_DIRECTIVE(sections)
+__OMP_DIRECTIVE(section)
+__OMP_DIRECTIVE(single)
+__OMP_DIRECTIVE(master)
+__OMP_DIRECTIVE(critical)
+__OMP_DIRECTIVE(taskyield)
+__OMP_DIRECTIVE(barrier)
+__OMP_DIRECTIVE(taskwait)
+__OMP_DIRECTIVE(taskgroup)
+__OMP_DIRECTIVE(flush)
+__OMP_DIRECTIVE(ordered)
+__OMP_DIRECTIVE(atomic)
+__OMP_DIRECTIVE(target)
+__OMP_DIRECTIVE(teams)
+__OMP_DIRECTIVE(cancel)
+__OMP_DIRECTIVE(requires)
+__OMP_DIRECTIVE_EXT(target_data, "target data")
+__OMP_DIRECTIVE_EXT(target_enter_data, "target enter data")
+__OMP_DIRECTIVE_EXT(target_exit_data, "target exit data")
+__OMP_DIRECTIVE_EXT(target_parallel, "target parallel")
+__OMP_DIRECTIVE_EXT(target_parallel_for, "target parallel for")
+__OMP_DIRECTIVE_EXT(target_update, "target update")
+__OMP_DIRECTIVE_EXT(parallel_for, "parallel for")
+__OMP_DIRECTIVE_EXT(parallel_for_simd, "parallel for simd")
+__OMP_DIRECTIVE_EXT(parallel_sections, "parallel sections")
+__OMP_DIRECTIVE_EXT(for_simd, "for simd")
+__OMP_DIRECTIVE_EXT(cancellation_point, "cancellation point")