subject:"\[PATCH\] D131915\: \[MLIR\]\[OpenMP\] Added target data, exit data, and enter data operation definition for MLIR."

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-23 Thread Akash Banerjee via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rGa4699a43e426: [MLIR][OpenMP] Added target data, exit data, 
and enter data operation… (authored by TIFitis).

Changed prior to commit:
  https://reviews.llvm.org/D131915?vs=489881=491299#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -495,6 +495,26 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32, %device_ptr: memref, %device_addr: memref, %map1: memref, %map2: memref) -> () {
+// CHECK: omp.target_data if(%[[VAL_0:.*]] : i1) device(%[[VAL_1:.*]] : si32) map((always, from -> %[[VAL_2:.*]] : memref))
+omp.target_data if(%if_cond : i1) device(%device : si32) map((always, from -> %map1 : memref)){}
+
+// CHECK: omp.target_data use_device_ptr(%[[VAL_3:.*]] : memref) use_device_addr(%[[VAL_4:.*]] : memref) map((close, present, to -> %[[VAL_2:.*]] : memref))
+omp.target_data use_device_ptr(%device_ptr : memref) use_device_addr(%device_addr : memref) map((close, present, to -> %map1 : memref)){}
+
+// CHECK: omp.target_data map((tofrom -> %[[VAL_2]] : memref), (alloc -> %[[VAL_5:.*]] : memref))
+omp.target_data map((tofrom -> %map1 : memref), (alloc -> %map2 : memref)){}
+
+// CHECK: omp.target_enter_data if(%[[VAL_0]] : i1) device(%[[VAL_1]] : si32) nowait map((alloc -> %[[VAL_2]] : memref))
+omp.target_enter_data if(%if_cond : i1) device(%device : si32) nowait map((alloc -> %map1 : memref))
+
+// CHECK: omp.target_exit_data if(%[[VAL_0]] : i1) device(%[[VAL_1]] : si32) nowait map((release -> %[[VAL_5]] : memref))
+omp.target_exit_data if(%if_cond : i1) device(%device : si32) nowait map((release -> %map2 : memref))
+
+return
+}
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
===
--- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -22,6 +22,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include 
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
@@ -552,6 +553,191 @@
   return success();
 }
 
+//===--===//
+// Parser, printer and verifier for Target Data
+//===--===//
+/// Parses a Map Clause.
+///
+/// map-clause = `map (` ( `(` `always, `? `close, `? `present, `? ( `to` |
+/// `from` | `delete` ) ` -> ` symbol-ref ` : ` type(symbol-ref) `)` )+ `)`
+/// Eg: map((release -> %1 : !llvm.ptr>), (always, close, from
+/// -> %2 : !llvm.ptr>))
+static ParseResult
+parseMapClause(OpAsmParser ,
+   SmallVectorImpl _operands,
+   SmallVectorImpl _operand_types, ArrayAttr _types) {
+  StringRef mapTypeMod;
+  OpAsmParser::UnresolvedOperand arg1;
+  Type arg1Type;
+  IntegerAttr arg2;
+  SmallVector mapTypesVec;
+  llvm::omp::OpenMPOffloadMappingFlags mapTypeBits;
+
+  auto parseTypeAndMod = [&]() -> ParseResult {
+if (parser.parseKeyword())
+  return failure();
+
+if (mapTypeMod == "always")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
+if (mapTypeMod == "close")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_CLOSE;
+if (mapTypeMod == "present")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PRESENT;
+
+if (mapTypeMod == "to")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
+if (mapTypeMod == "from")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+if (mapTypeMod == "tofrom")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
+ llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+if (mapTypeMod == "delete")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
+return success();
+  };
+
+  auto parseMap = [&]() -> ParseResult {
+mapTypeBits = llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
+
+if (parser.parseLParen() ||
+parser.parseCommaSeparatedList(parseTypeAndMod) ||
+parser.parseArrow() || parser.parseOperand(arg1) ||
+parser.parseColon() || parser.parseType(arg1Type) ||
+

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-20 Thread Valentin Clement via Phabricator via cfe-commits

clementval added a comment.

Just small nit




Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:628-629
+int64_t mapTypeBits = 0x00;
+auto mapOp = map_operands[i];
+auto mapTypeOp = map_types[i];
+

auto should be spelled out here. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-20 Thread Akash Banerjee via Phabricator via cfe-commits

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

I'm planning on merging the patch early next week. If you'd like to see any 
changes or need more time to review, please let me know :)

Akash


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-17 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis marked 5 inline comments as done.
TIFitis added a comment.

In D131915#4056730 , 
@kiranchandramohan wrote:

> LGTM. Thanks for making the changes and for your patience. Please wait a 
> couple of days to give other reviewers a chance to have a look before 
> submitting.

Thanks for reviewing the changes as well :)

> Could you update the Summary of this patch to specify what is in this patch 
> and what is left out? Also, might be useful to specify any special modelling, 
> like for the map clause.

I've updated the summary. The new map clause looks straight forward to me, 
anything particular you want me to mention in the summary?

> Could you specify the co-authors in the following way?
> Co-authored-by: abidmalikwaterloo 
> Co-authored-by: raghavendra 

Done.

Cheers,
Akash




Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:790
+//===-===//
+// 2.12.2 target data Construct
+//===-===//

kiranchandramohan wrote:
> Is this number from the OpenMP 5.0 standard? I think 5.0 does not have the 
> `present` map-type modifier. The printer includes this. I think we can either 
> remove things that are not there in 5.0 or add comments when items from newer 
> versions are included or alternatively change the number to the latest 
> version and call out everything that is not implemented.
Hi I've updated it to OpenMP 5.1 standard specification. We are missing the 
`depend` clause, and mapper and iterator values for the map_type_modifier which 
are already specified in the TODO.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:542
+// Parser, printer and verifier for Target Data
+//===--===//
+static ParseResult

kiranchandramohan wrote:
> Could you specify the EBNF (https://mlir.llvm.org/docs/LangRef/#notation) for 
> the expected structure of the map clause?
Now that we are using `IntegerAttr` we no longer need the Attr to be explicitly 
present when printing it. 

I have thus updated the printer and parser accordingly, also removed `none` as 
a map_type_modifier as absence of other modifiers implicitly implies it and 
prevents confusion by diverging from the specification.

Here's an example of the new custom printer:

```
omp.target_exit_data   map((release -> %1 : !llvm.ptr>), 
(always, close, from -> %2 : !llvm.ptr>))
```

Let me know if the EBNF is okay. Should I override clang-format and keep the 
grammar and Eg in the same line to make them easier to read?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-17 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis updated this revision to Diff 489881.
TIFitis added a comment.

Addressed reviewer comments.
Updated printer and parser to remove the IntegerAttr from appearing. Removed 
none from map type modifiers, absence of other modifiers implicitly means none. 
This helps keep it in closer to the specification.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -451,6 +451,26 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32, %device_ptr: memref, %device_addr: memref, %map1: memref, %map2: memref) -> () {
+// CHECK: omp.target_data if(%[[VAL_0:.*]] : i1) device(%[[VAL_1:.*]] : si32) map((always, from -> %[[VAL_2:.*]] : memref))
+omp.target_data if(%if_cond : i1) device(%device : si32) map((always, from -> %map1 : memref)){}
+
+// CHECK: omp.target_data use_device_ptr(%[[VAL_3:.*]] : memref) use_device_addr(%[[VAL_4:.*]] : memref) map((close, present, to -> %[[VAL_2:.*]] : memref))
+omp.target_data use_device_ptr(%device_ptr : memref) use_device_addr(%device_addr : memref) map((close, present, to -> %map1 : memref)){}
+
+// CHECK: omp.target_data map((tofrom -> %[[VAL_2]] : memref), (alloc -> %[[VAL_5:.*]] : memref))
+omp.target_data map((tofrom -> %map1 : memref), (alloc -> %map2 : memref)){}
+
+// CHECK: omp.target_enter_data if(%[[VAL_0]] : i1) device(%[[VAL_1]] : si32) nowait map((alloc -> %[[VAL_2]] : memref))
+omp.target_enter_data if(%if_cond : i1) device(%device : si32) nowait map((alloc -> %map1 : memref))
+
+// CHECK: omp.target_exit_data if(%[[VAL_0]] : i1) device(%[[VAL_1]] : si32) nowait map((release -> %[[VAL_5]] : memref))
+omp.target_exit_data if(%if_cond : i1) device(%device : si32) nowait map((release -> %map2 : memref))
+
+return
+}
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
===
--- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -22,6 +22,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include 
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
@@ -536,6 +537,191 @@
   return success();
 }
 
+//===--===//
+// Parser, printer and verifier for Target Data
+//===--===//
+/// Parses a Map Clause.
+///
+/// map-clause = `map (` ( `(` `always, `? `close, `? `present, `? ( `to` |
+/// `from` | `delete` ) ` -> ` symbol-ref ` : ` type(symbol-ref) `)` )+ `)`
+/// Eg: map((release -> %1 : !llvm.ptr>), (always, close, from
+/// -> %2 : !llvm.ptr>))
+static ParseResult
+parseMapClause(OpAsmParser ,
+   SmallVectorImpl _operands,
+   SmallVectorImpl _operand_types, ArrayAttr _types) {
+  StringRef mapTypeMod;
+  OpAsmParser::UnresolvedOperand arg1;
+  Type arg1Type;
+  IntegerAttr arg2;
+  SmallVector mapTypesVec;
+  llvm::omp::OpenMPOffloadMappingFlags mapTypeBits;
+
+  auto parseTypeAndMod = [&]() -> ParseResult {
+if (parser.parseKeyword())
+  return failure();
+
+if (mapTypeMod == "always")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_ALWAYS;
+if (mapTypeMod == "close")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_CLOSE;
+if (mapTypeMod == "present")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_PRESENT;
+
+if (mapTypeMod == "to")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
+if (mapTypeMod == "from")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+if (mapTypeMod == "tofrom")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO |
+ llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_FROM;
+if (mapTypeMod == "delete")
+  mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_DELETE;
+return success();
+  };
+
+  auto parseMap = [&]() -> ParseResult {
+mapTypeBits = llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
+
+if (parser.parseLParen() ||
+parser.parseCommaSeparatedList(parseTypeAndMod) ||
+parser.parseArrow() || parser.parseOperand(arg1) ||
+parser.parseColon() ||

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-16 Thread Kiran Chandramohan via Phabricator via cfe-commits

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

LGTM. Thanks for making the changes and for your patience. Please wait a couple 
of days to give other reviewers a chance to have a look before submitting.

Could you update the Summary of this patch to specify what is in this patch and 
what is left out? Also, might be useful to specify any special modelling, like 
for the map clause.

Could you specify the co-authors in the following way?
Co-authored-by: abidmalikwaterloo 
Co-authored-by: raghavendra 




Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:790
+//===-===//
+// 2.12.2 target data Construct
+//===-===//

Is this number from the OpenMP 5.0 standard? I think 5.0 does not have the 
`present` map-type modifier. The printer includes this. I think we can either 
remove things that are not there in 5.0 or add comments when items from newer 
versions are included or alternatively change the number to the latest version 
and call out everything that is not implemented.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:818-820
+The $map_operands specifies operands in map clause along with it's type 
and modifiers.
+
+The $map_operand_segments specifies the segment sizes for $map_operands.

Update these two to their current meanings. (Also replace 
`map_operand_segments` with `map_types`.

Same comment for the other two operations.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:542
+// Parser, printer and verifier for Target Data
+//===--===//
+static ParseResult

Could you specify the EBNF (https://mlir.llvm.org/docs/LangRef/#notation) for 
the expected structure of the map clause?



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:595
+
+if (mapTypeOp.isa())
+  mapTypeBits = mapTypeOp.cast().getInt();

Nit: Should this be an assert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-16 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis updated this revision to Diff 489515.
TIFitis added a comment.

Minor indentation changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -451,6 +451,26 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32, %device_ptr: memref, %device_addr: memref, %map1: memref, %map2: memref) -> () {
+// CHECK: omp.target_data if(%[[VAL_0:.*]] : i1) device(%[[VAL_1:.*]] : si32) map((6 : i64 -> always , from : %[[VAL_2:.*]] : memref))
+omp.target_data if(%if_cond : i1) device(%device : si32) map((6 -> always , from : %map1 : memref)){}
+
+// CHECK: omp.target_data use_device_ptr(%[[VAL_3:.*]] : memref) use_device_addr(%[[VAL_4:.*]] : memref) map((6 : i64 -> always , from : %[[VAL_2:.*]] : memref))
+omp.target_data use_device_ptr(%device_ptr : memref) use_device_addr(%device_addr : memref) map((6 -> always , from : %map1 : memref)){}
+
+// CHECK: omp.target_data map((6 : i64 -> always , from : %[[VAL_2]] : memref), (0 : i64 -> none , alloc : %[[VAL_5:.*]] : memref))
+omp.target_data map((6 -> always , from : %map1 : memref), (0 -> none , alloc : %map2 : memref)){}
+
+// CHECK: omp.target_enter_data if(%[[VAL_0]] : i1) device(%[[VAL_1]] : si32) nowait map((0 : i64 -> none , alloc : %[[VAL_2]] : memref))
+omp.target_enter_data if(%if_cond : i1) device(%device : si32) nowait map((0 -> none , alloc : %map1 : memref))
+
+// CHECK: omp.target_exit_data if(%[[VAL_0]] : i1) device(%[[VAL_1]] : si32) nowait map((0 : i64 -> none , release : %[[VAL_5]] : memref))
+omp.target_exit_data if(%if_cond : i1) device(%device : si32) nowait map((0 -> none , release : %map2 : memref))
+
+return
+}
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
===
--- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -22,6 +22,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include 
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
@@ -536,6 +537,157 @@
   return success();
 }
 
+//===--===//
+// Parser, printer and verifier for Target Data
+//===--===//
+static ParseResult
+parseMapClause(OpAsmParser ,
+   SmallVectorImpl _operands,
+   SmallVectorImpl _operand_types, ArrayAttr _types) {
+  StringRef mapTypeMod, mapType;
+  OpAsmParser::UnresolvedOperand arg1;
+  IntegerAttr arg2;
+  Type arg2Type;
+  SmallVector mapTypesVec;
+
+  auto parseKeyword = [&]() -> ParseResult {
+if (parser.parseLParen() || parser.parseAttribute(arg2) ||
+parser.parseArrow() || parser.parseKeyword() ||
+parser.parseComma() || parser.parseKeyword() ||
+parser.parseColon() || parser.parseOperand(arg1) ||
+parser.parseColon() || parser.parseType(arg2Type) ||
+parser.parseRParen())
+  return failure();
+map_operands.push_back(arg1);
+map_operand_types.push_back(arg2Type);
+mapTypesVec.push_back(arg2);
+return success();
+  };
+
+  if (parser.parseCommaSeparatedList(parseKeyword))
+return failure();
+
+  SmallVector mapTypesAttr(mapTypesVec.begin(), mapTypesVec.end());
+  map_types = ArrayAttr::get(parser.getContext(), mapTypesAttr);
+  return success();
+}
+
+static void printMapClause(OpAsmPrinter , Operation *op,
+   OperandRange map_operands,
+   TypeRange map_operand_types, ArrayAttr map_types) {
+
+  // Helper function to get bitwise AND of `value` and 'flag'
+  auto bitAnd = [](int64_t value,
+   llvm::omp::OpenMPOffloadMappingFlags flag) -> bool {
+return value &
+   static_cast<
+   std::underlying_type_t>(
+   flag);
+  };
+
+  assert(map_operands.size() == map_types.size());
+
+  for (unsigned i = 0, e = map_operands.size(); i < e; i++) {
+int64_t mapTypeBits = 0x00;
+auto mapOp = map_operands[i];
+auto mapTypeOp = map_types[i];
+
+if (mapTypeOp.isa())
+  mapTypeBits = mapTypeOp.cast().getInt();
+
+bool always = bitAnd(mapTypeBits,
+

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-16 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis updated this revision to Diff 489510.
TIFitis added a comment.

Removed use of VariadicOfVariadic. Added new I64ArrayAttr for map_types.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -451,6 +451,26 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32, %device_ptr: memref, %device_addr: memref, %map1: memref, %map2: memref) -> () {
+// CHECK: omp.target_data if(%[[VAL_0:.*]] : i1) device(%[[VAL_1:.*]] : si32) map((6 : i64 -> always , from : %[[VAL_2:.*]] : memref))
+omp.target_data if(%if_cond : i1) device(%device : si32) map((6 -> always , from : %map1 : memref)){}
+
+// CHECK: omp.target_data use_device_ptr(%[[VAL_3:.*]] : memref) use_device_addr(%[[VAL_4:.*]] : memref) map((6 : i64 -> always , from : %[[VAL_2:.*]] : memref))
+omp.target_data use_device_ptr(%device_ptr : memref) use_device_addr(%device_addr : memref) map((6 -> always , from : %map1 : memref)){}
+
+// CHECK: omp.target_data map((6 : i64 -> always , from : %[[VAL_2]] : memref), (0 : i64 -> none , alloc : %[[VAL_5:.*]] : memref))
+omp.target_data map((6 -> always , from : %map1 : memref), (0 -> none , alloc : %map2 : memref)){}
+
+// CHECK: omp.target_enter_data if(%[[VAL_0]] : i1) device(%[[VAL_1]] : si32) nowait map((0 : i64 -> none , alloc : %[[VAL_2]] : memref))
+omp.target_enter_data if(%if_cond : i1) device(%device : si32) nowait map((0 -> none , alloc : %map1 : memref))
+
+// CHECK: omp.target_exit_data if(%[[VAL_0]] : i1) device(%[[VAL_1]] : si32) nowait map((0 : i64 -> none , release : %[[VAL_5]] : memref))
+omp.target_exit_data if(%if_cond : i1) device(%device : si32) nowait map((0 -> none , release : %map2 : memref))
+
+return
+}
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
===
--- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -22,6 +22,7 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include 
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
@@ -536,6 +537,154 @@
   return success();
 }
 
+//===--===//
+// Parser, printer and verifier for Target Data
+//===--===//
+static ParseResult
+parseMapClause(OpAsmParser ,
+   SmallVectorImpl _operands,
+   SmallVectorImpl _operand_types, ArrayAttr _types) {
+  StringRef mapTypeMod, mapType;
+  OpAsmParser::UnresolvedOperand arg1;
+  IntegerAttr arg2;
+  Type arg2Type;
+  SmallVector mapTypesVec;
+  auto parseKeyword = [&]() -> ParseResult {
+if (parser.parseLParen() || parser.parseAttribute(arg2) ||
+parser.parseArrow() || parser.parseKeyword() ||
+parser.parseComma() || parser.parseKeyword() ||
+parser.parseColon() || parser.parseOperand(arg1) ||
+parser.parseColon() || parser.parseType(arg2Type) ||
+parser.parseRParen())
+  return failure();
+map_operands.push_back(arg1);
+map_operand_types.push_back(arg2Type);
+mapTypesVec.push_back(arg2);
+return success();
+  };
+  if (parser.parseCommaSeparatedList(parseKeyword))
+return failure();
+  SmallVector mapTypesAttr(mapTypesVec.begin(), mapTypesVec.end());
+  map_types = ArrayAttr::get(parser.getContext(), mapTypesAttr);
+  return success();
+}
+
+static void printMapClause(OpAsmPrinter , Operation *op,
+   OperandRange map_operands,
+   TypeRange map_operand_types, ArrayAttr map_types) {
+
+  // Helper function to get bitwise AND of `value` and 'flag'
+  auto bitAnd = [](int64_t value,
+   llvm::omp::OpenMPOffloadMappingFlags flag) -> bool {
+return value &
+   static_cast<
+   std::underlying_type_t>(
+   flag);
+  };
+
+  assert(map_operands.size() == map_types.size());
+
+  for (unsigned i = 0, e = map_operands.size(); i < e; i++) {
+int64_t mapTypeBits = 0x00;
+auto mapOp = map_operands[i];
+auto mapTypeOp = map_types[i];
+
+if (mapTypeOp.isa())
+  mapTypeBits = mapTypeOp.cast().getInt();
+
+bool always = bitAnd(mapTypeBits,
+

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-15 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan added inline comments.



Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > Why is this needed here?
> > > The Arith Dialect needs to be linked against as we're using it extract 
> > > the int value from arith.constant in the custom printer.
> > Can this be avoided by modelling constants as attributes?
> The issue with attributes is AFAIK `Variadic` is not supported, 
> and as previously discussed we need it be `Variadic` to support multiple map 
> clauses.
> 
> If I am wrong and there is indeed a way to have `Variadic` then 
> this can be avoided.
Can we use an ArrayAttr 
(https://mlir.llvm.org/docs/Dialects/Builtin/#arrayattr) or something derived 
from it?
https://github.com/llvm/llvm-project/blob/6ee5a1a090f3f4b6ae7ec9915900023277daf8d7/mlir/include/mlir/IR/OpBase.td#L1467


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-15 Thread Akash Banerjee via Phabricator via cfe-commits

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



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:829
+  VariadicOfVariadic:$map_operands,
+  DenseI32ArrayAttr:$map_operand_segments);
+

kiranchandramohan wrote:
> Is `map_operand_segments` currently unused?
`map_operand_segments` is required by VariadicOfVariadic type to keep track of 
the dimensions. I haven't made explicit use of it, however, it is used 
internally and cant be avoided AFAIK. 

It is akin to the `operand_segment_sizes` which is implicitly present for 
Operations with `Optional` or `Variadic` operands.



Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )

kiranchandramohan wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > Why is this needed here?
> > The Arith Dialect needs to be linked against as we're using it extract the 
> > int value from arith.constant in the custom printer.
> Can this be avoided by modelling constants as attributes?
The issue with attributes is AFAIK `Variadic` is not supported, and 
as previously discussed we need it be `Variadic` to support multiple map 
clauses.

If I am wrong and there is indeed a way to have `Variadic` then this 
can be avoided.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:577-583
+if (auto constOp =
+mlir::dyn_cast(a.front().getDefiningOp()))
+  mapTypeBits = constOp.getValue()
+.cast()
+.getValue()
+.getSExtValue();
+else if (auto constOp = mlir::dyn_cast(

kiranchandramohan wrote:
> Generally, constant values are modelled as attributes in MLIR representation. 
> Can we switch to that representation? This will also avoid the need for this 
> `if-else` and dependence on the `arith` dialect.
Copy:
The issue with attributes is AFAIK `Variadic` is not supported, and 
as previously discussed we need it be `Variadic` to support multiple map 
clauses.

If I am wrong and there is indeed a way to have `Variadic` then this 
can be avoided.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2023-01-15 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis updated this revision to Diff 489345.
TIFitis marked 7 inline comments as done.
TIFitis added a comment.

Added verifiers, removed use of magic constants and addressed other reviewer 
comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/CMakeLists.txt
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -451,6 +451,32 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32, %device_ptr: memref, %device_addr: memref, %map1: memref, %map2: memref) -> () {
+// CHECK: %[[VAL_0:.*]] = arith.constant 0 : i64
+%c0_i64 = arith.constant 0 : i64
+
+// CHECK: %[[VAL_1:.*]] = arith.constant 6 : i64
+%c6_i64 = arith.constant 6 : i64
+
+// CHECK: omp.target_data if(%[[VAL_2:.*]] : i1) device(%[[VAL_3:.*]] : si32) map((%[[VAL_1]] -> always , from : %[[VAL_4:.*]] : memref))
+omp.target_data if(%if_cond : i1) device(%device : si32) map((%c6_i64 -> always , from : %map1 : memref)){}
+
+// CHECK: omp.target_data use_device_ptr(%[[VAL_5:.*]] : memref) use_device_addr(%[[VAL_6:.*]] : memref) map((%[[VAL_1]] -> always , from : %[[VAL_4]] : memref))
+omp.target_data use_device_ptr(%device_ptr : memref) use_device_addr(%device_addr : memref) map((%c6_i64 -> always , from : %map1 : memref)){}
+
+// CHECK: omp.target_data map((%[[VAL_1]] -> always , from : %[[VAL_4]] : memref), (%[[VAL_0]] -> none , alloc : %[[VAL_7:.*]] : memref))
+omp.target_data map((%c6_i64 -> always , from : %map1 : memref), (%c0_i64 -> none , alloc : %map2 : memref)){}
+
+// CHECK: omp.target_enter_data if(%[[VAL_2]] : i1) device(%[[VAL_3]] : si32) nowait map((%[[VAL_0]] -> none , alloc : %[[VAL_7]] : memref))
+omp.target_enter_data if(%if_cond : i1) device(%device : si32) nowait map((%c0_i64 -> none , alloc : %map2 : memref))
+
+// CHECK: omp.target_exit_data if(%[[VAL_2]] : i1) device(%[[VAL_3]] : si32) nowait map((%[[VAL_0]] -> none , release : %[[VAL_7]] : memref))
+omp.target_exit_data if(%if_cond : i1) device(%device : si32) nowait map((%c0_i64 -> none , release : %map2 : memref))
+
+return
+}
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
===
--- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "mlir/Dialect/Arith/IR/Arith.h"
 #include "mlir/Dialect/LLVMIR/LLVMTypes.h"
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/DialectImplementation.h"
@@ -22,7 +23,9 @@
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TypeSwitch.h"
+#include "llvm/Frontend/OpenMP/OMPConstants.h"
 #include 
+#include 
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
 #include "mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc"
@@ -536,6 +539,159 @@
   return success();
 }
 
+//===--===//
+// Parser, printer and verifier for Target Data
+//===--===//
+static ParseResult parseMapClause(
+OpAsmParser ,
+SmallVector> _operands,
+SmallVector> _operand_types) {
+  StringRef mapTypeMod, mapType;
+  OpAsmParser::UnresolvedOperand arg1, arg2;
+  Type arg2Type;
+  auto parseKeyword = [&]() -> ParseResult {
+if (parser.parseLParen() || parser.parseOperand(arg1) ||
+parser.parseArrow() || parser.parseKeyword() ||
+parser.parseComma() || parser.parseKeyword() ||
+parser.parseColon() || parser.parseOperand(arg2) ||
+parser.parseColon() || parser.parseType(arg2Type) ||
+parser.parseRParen())
+  return failure();
+map_operands.push_back({arg1, arg2});
+map_operand_types.push_back({parser.getBuilder().getI64Type(), arg2Type});
+return success();
+  };
+  if (parser.parseCommaSeparatedList(parseKeyword))
+return failure();
+  return success();
+}
+
+static void printMapClause(OpAsmPrinter , Operation *op,
+   OperandRangeRange map_operands,
+   TypeRangeRange map_operand_types) {
+
+  // Helper function to get bitwise AND of `value` and 'flag'
+  auto bitAnd = [](int64_t value,
+   llvm::omp::OpenMPOffloadMappingFlags flag)

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-16 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Can you add the other authors as Co-authors in the patch Summary?

In D131915#4001620 , @TIFitis wrote:

>> Can you add this as a test? AFAIS, the tests attached to this patch do not 
>> seem to be exercising the `VariadicofVariadic` requirement. An explanation 
>> with an example would be great.
>
> `VariadicOfVariadic` gives us a `SmallVector` where `ValueRange` 
> is essentially a `SmallVector`. As it is possible to have multiple map 
> clauses for a single target construct, this allows us to represent each map 
> clause as a row in `$map_operands`.
>
> I've updated the ops.mlir test to use two map clauses which better shows this 
> use case.
>
>> 1. Fortran Source + OpenMP example that needs a VariadicOfVariadic
>> 2. MLIR OpenMP representation
>
> Fortran Source:
>
>   subroutine omp_target_enter
>  integer :: a(1024)
>  integer :: b(1024)
>  !$omp target enter data map(to: a,) map(always, alloc: b)
>   end subroutine omp_target_enter
>
> MLIR OpenMP representation:
>
>   func.func @_QPomp_target_enter() {
> %0 = fir.alloca !fir.array<1024xi32> {bindc_name = "a", uniq_name = 
> "_QFomp_target_enterEa"}
> %1 = fir.alloca !fir.array<1024xi32> {bindc_name = "b", uniq_name = 
> "_QFomp_target_enterEb"}
> %c1_i64 = arith.constant 1 : i64
> %c4_i64 = arith.constant 4 : i64
> omp.target_enter_data   map((%c1_i64 -> none , to : %0 : 
> !fir.ref>), (%c4_i64 -> always , alloc : %1 : 
> !fir.ref>))
> return
>   }

Thanks, that helps.

> I plan on adding these as tests along with Fortran lowering support in 
> upcoming patches.
>
>> I am assuming we would need a verifier as well for the map clause.
>
> AFAIK the map clause rules are op specific. I plan on adding verifiers for 
> the ops soon in a separate patch.
>
>> Can you also restrict this patch to one of the constructs say `target data` 
>> for this patch? Once we decide on that then the other two can be easily 
>> added in a separate patch.
>
> Since I didn't make any changes to the ops I've left them in. If the patch 
> requires further changes, I'll leave them out.

The concern is with the introduction of VariadicOfVariadic with `AnyType`. I 
think this is new in the OpenMP Dialect and it pretty much allows anything. So, 
I was thinking whether it would be a good idea to write a verifier for the map 
clause, if not for the entire construct.




Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:826-827
+  Optional:$device,
+  Variadic:$use_device_ptr,
+  Variadic:$use_device_addr,
+  VariadicOfVariadic:$map_operands,

This is OK for now, but we might have to switch to `OpenMPPointerType` later.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:829
+  VariadicOfVariadic:$map_operands,
+  DenseI32ArrayAttr:$map_operand_segments);
+

Is `map_operand_segments` currently unused?



Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )

TIFitis wrote:
> kiranchandramohan wrote:
> > Why is this needed here?
> The Arith Dialect needs to be linked against as we're using it extract the 
> int value from arith.constant in the custom printer.
Can this be avoided by modelling constants as attributes?



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:577-583
+if (auto constOp =
+mlir::dyn_cast(a.front().getDefiningOp()))
+  mapTypeBits = constOp.getValue()
+.cast()
+.getValue()
+.getSExtValue();
+else if (auto constOp = mlir::dyn_cast(

Generally, constant values are modelled as attributes in MLIR representation. 
Can we switch to that representation? This will also avoid the need for this 
`if-else` and dependence on the `arith` dialect.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:595
+
+std::stringstream typeMod, type;
+if (always)

I think llvm prefers using `llvm::raw_string_ostream`. I have seen only two 
uses of std::stringstream in MLIR code, possibly accidentally introduced.



Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+// CHECK: omp.target_data
+"omp.target_data"(%if_cond, %device, %data1, %data2) ({
+   

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > clementval wrote:
> > > > Can you switch to the custom format form?
> > > Hi, I've updated the test file, Is this what you wanted?
> > Both the input and the CHECK lines in the custom format.
> I've changed both to custom format. Added a second map clause argument to 
> show support.
Can you increase the coverage

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-16 Thread Valentin Clement via Phabricator via cfe-commits

clementval added inline comments.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:584
+std::stringstream typeMod, type;
+if (mapTypeBits & 0x04)
+  typeMod << "always ";

jdoerfert wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > There is a `bitn` function in `printSynchronizationHint` and 
> > > `verifySynchronizationHint`, can that be used here?
> > I've added something similar. I got the bit values from Clang codegen and 
> > they use hex so I've kept it that way for uniformity. Let me know if you'd 
> > rather it be in n'th bit format.
> Clang codegen magic constants (and everything else) is moved to 
> llvm/.../Frontend/OpenMP for a reason. Hardcoding the numbers again somewhere 
> else seems like exactly the wrong thing to do.
+1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-16 Thread Johannes Doerfert via Phabricator via cfe-commits

jdoerfert added inline comments.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:584
+std::stringstream typeMod, type;
+if (mapTypeBits & 0x04)
+  typeMod << "always ";

TIFitis wrote:
> kiranchandramohan wrote:
> > There is a `bitn` function in `printSynchronizationHint` and 
> > `verifySynchronizationHint`, can that be used here?
> I've added something similar. I got the bit values from Clang codegen and 
> they use hex so I've kept it that way for uniformity. Let me know if you'd 
> rather it be in n'th bit format.
Clang codegen magic constants (and everything else) is moved to 
llvm/.../Frontend/OpenMP for a reason. Hardcoding the numbers again somewhere 
else seems like exactly the wrong thing to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-16 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis marked 3 inline comments as done.
TIFitis added a comment.

> Can you add this as a test? AFAIS, the tests attached to this patch do not 
> seem to be exercising the `VariadicofVariadic` requirement. An explanation 
> with an example would be great.

`VariadicOfVariadic` gives us a `SmallVector` where `ValueRange` is 
essentially a `SmallVector`. As it is possible to have multiple map 
clauses for a single target construct, this allows us to represent each map 
clause as a row in `$map_operands`.

I've updated the ops.mlir test to use two map clauses which better shows this 
use case.

> 1. Fortran Source + OpenMP example that needs a VariadicOfVariadic
> 2. MLIR OpenMP representation

Fortran Source:

  subroutine omp_target_enter
 integer :: a(1024)
 integer :: b(1024)
 !$omp target enter data map(to: a,) map(always, alloc: b)
  end subroutine omp_target_enter

MLIR OpenMP representation:

  func.func @_QPomp_target_enter() {
%0 = fir.alloca !fir.array<1024xi32> {bindc_name = "a", uniq_name = 
"_QFomp_target_enterEa"}
%1 = fir.alloca !fir.array<1024xi32> {bindc_name = "b", uniq_name = 
"_QFomp_target_enterEb"}
%c1_i64 = arith.constant 1 : i64
%c4_i64 = arith.constant 4 : i64
omp.target_enter_data   map((%c1_i64 -> none , to : %0 : 
!fir.ref>), (%c4_i64 -> always , alloc : %1 : 
!fir.ref>))
return
  }

I plan on adding these as tests along with Fortran lowering support in upcoming 
patches.

> I am assuming we would need a verifier as well for the map clause.

AFAIK the map clause rules are op specific. I plan on adding verifiers for the 
ops soon in a separate patch.

> Can you also restrict this patch to one of the constructs say `target data` 
> for this patch? Once we decide on that then the other two can be easily added 
> in a separate patch.

Since I didn't make any changes to the ops I've left them in. If the patch 
requires further changes, I'll leave them out.




Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )

kiranchandramohan wrote:
> Why is this needed here?
The Arith Dialect needs to be linked against as we're using it extract the int 
value from arith.constant in the custom printer.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:584
+std::stringstream typeMod, type;
+if (mapTypeBits & 0x04)
+  typeMod << "always ";

kiranchandramohan wrote:
> There is a `bitn` function in `printSynchronizationHint` and 
> `verifySynchronizationHint`, can that be used here?
I've added something similar. I got the bit values from Clang codegen and they 
use hex so I've kept it that way for uniformity. Let me know if you'd rather it 
be in n'th bit format.



Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+// CHECK: omp.target_data
+"omp.target_data"(%if_cond, %device, %data1, %data2) ({
+   

kiranchandramohan wrote:
> TIFitis wrote:
> > clementval wrote:
> > > Can you switch to the custom format form?
> > Hi, I've updated the test file, Is this what you wanted?
> Both the input and the CHECK lines in the custom format.
I've changed both to custom format. Added a second map clause argument to show 
support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-16 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis updated this revision to Diff 483545.
TIFitis marked 2 inline comments as done.
TIFitis added a comment.

Addresed reviewer comments.
Added second map clause to test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/CMakeLists.txt
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -451,6 +451,27 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32, %map1: memref, %map2: memref) -> () {
+// CHECK: %[[VAL_0:.*]] = arith.constant 0 : i64
+%c0_i64 = arith.constant 0 : i64
+
+// CHECK: %[[VAL_1:.*]] = arith.constant 6 : i64
+%c6_i64 = arith.constant 6 : i64
+
+// CHECK: omp.target_data map((%[[VAL_1]] -> always , from : %[[VAL_4:.*]] : memref), (%[[VAL_0]] -> none , release : %[[VAL_5:.*]] : memref))
+omp.target_data map((%c6_i64 -> always , from : %map1 : memref), (%c0_i64 -> none , release : %map2 : memref)){}
+
+// CHECK: omp.target_enter_data if(%[[VAL_2:.*]] : i1) device(%[[VAL_3:.*]] : si32) map((%[[VAL_0]] -> none , alloc : %[[VAL_5:.*]] : memref))
+omp.target_enter_data if(%if_cond : i1) device(%device : si32) map((%c0_i64 -> none , alloc : %map2 : memref)){}
+
+// CHECK: omp.target_exit_data if(%[[VAL_2:.*]] : i1) device(%[[VAL_3:.*]] : si32) map((%[[VAL_0]] -> none , release : %[[VAL_5:.*]] : memref))
+omp.target_exit_data if(%if_cond : i1) device(%device : si32) map((%c0_i64 -> none , release : %map2 : memref)){}
+return
+}
+
+
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
===
--- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "mlir/Dialect/Arith/IR/Arith.h"
 #include "mlir/Dialect/LLVMIR/LLVMTypes.h"
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/DialectImplementation.h"
@@ -23,6 +24,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TypeSwitch.h"
 #include 
+#include 
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
 #include "mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc"
@@ -536,6 +538,86 @@
   return success();
 }
 
+//===--===//
+// Parser, printer and verifier for Target Data
+//===--===//
+static ParseResult parseMapClause(
+OpAsmParser ,
+SmallVector> _operands,
+SmallVector> _operand_types) {
+  StringRef mapTypeMod, mapType;
+  OpAsmParser::UnresolvedOperand arg1, arg2;
+  Type arg2Type;
+  auto parseKeyword = [&]() -> ParseResult {
+if (parser.parseLParen() || parser.parseOperand(arg1) ||
+parser.parseArrow() || parser.parseKeyword() ||
+parser.parseComma() || parser.parseKeyword() ||
+parser.parseColon() || parser.parseOperand(arg2) ||
+parser.parseColon() || parser.parseType(arg2Type) ||
+parser.parseRParen())
+  return failure();
+map_operands.push_back({arg1, arg2});
+map_operand_types.push_back({parser.getBuilder().getI64Type(), arg2Type});
+return success();
+  };
+  if (parser.parseCommaSeparatedList(parseKeyword))
+return failure();
+  return success();
+}
+
+static void printMapClause(OpAsmPrinter , Operation *op,
+   OperandRangeRange map_operands,
+   TypeRangeRange map_operand_types) {
+
+  // Helper function to get bitwise AND of `value` and 'n'
+  auto bitAnd = [](int64_t value, int64_t n) -> bool { return value & n; };
+
+  for (const auto  : map_operands) {
+int64_t mapTypeBits = 0x00;
+if (auto constOp =
+mlir::dyn_cast(a.front().getDefiningOp()))
+  mapTypeBits = constOp.getValue()
+.cast()
+.getValue()
+.getSExtValue();
+else if (auto constOp = mlir::dyn_cast(
+ a.front().getDefiningOp()))
+  mapTypeBits = constOp.getValue().dyn_cast().getInt();
+
+bool always = bitAnd(mapTypeBits, 0x04);
+bool close = bitAnd(mapTypeBits, 0x400);
+bool present = bitAnd(mapTypeBits, 0x1000);
+
+bool to = bitAnd(mapTypeBits, 0x01);
+bool from = bitAnd(mapTypeBits, 0x02);
+bool del = bitAnd(mapTypeBits, 0x08);
+
+

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-16 Thread Kiran Chandramohan via Phabricator via cfe-commits

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Thanks for the changes and your work here.

In D131915#3994160 , @TIFitis wrote:

> Added custom printer & parser for map clause. Updated ops.mlir test to 
> address reviewer comments.
>
> Custom Printer example:
>
> OMP: !$omp target exit data map(from: a,b) map(release: c) map(delete: d) 
> map(always, from: e)
> CustomPrinter: omp.target_exit_data   map((%c2_i64 -> none , from : %0 : 
> !fir.ref>), (%c2_i64 -> none , from : %1 : 
> !fir.ref>), (%c0_i64 -> none , release : %2 : 
> !fir.ref>), (%c8_i64 -> none , delete : %3 : 
> !fir.ref>), (%c6_i64 -> always , from : %4 : 
> !fir.ref>))

Can you add this as a test? AFAIS, the tests attached to this patch do not seem 
to be exercising the `VariadicofVariadic` requirement. An explanation with an 
example would be great.

1. Fortran Source + OpenMP example that needs a VariadicOfVariadic
2. MLIR OpenMP representation

I am assuming we would need a verifier as well for the map clause.

Can you also restrict this patch to one of the constructs say `target data` for 
this patch? Once we decide on that then the other two can be easily added in a 
separate patch.

Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )

Why is this needed here?

Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:572
+  for (const auto  : map_operands) {
+auto mapTypeBits = 0x00;
+if (auto constOp =

Nit: can you specify the type here?

Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:584
+std::stringstream typeMod, type;
+if (mapTypeBits & 0x04)
+  typeMod << "always ";

There is a `bitn` function in `printSynchronizationHint` and 
`verifySynchronizationHint`, can that be used here?

Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+// CHECK: omp.target_data
+"omp.target_data"(%if_cond, %device, %data1, %data2) ({
+   

TIFitis wrote:
> clementval wrote:
> > Can you switch to the custom format form?
> Hi, I've updated the test file, Is this what you wanted?
Both the input and the CHECK lines in the custom format.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-14 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis marked 10 inline comments as done.
TIFitis added inline comments.



Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+// CHECK: omp.target_data
+"omp.target_data"(%if_cond, %device, %data1, %data2) ({
+   

clementval wrote:
> Can you switch to the custom format form?
Hi, I've updated the test file, Is this what you wanted?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-14 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis updated this revision to Diff 482732.
TIFitis added a comment.

Added custom printer & parser for map clause. Updated ops.mlir test to address 
reviewer comments.

Custom Printer example:

OMP: !$omp target exit data map(from: a,b) map(release: c) map(delete: d) 
map(always, from: e)
CustomPrinter: omp.target_exit_data   map((%c2_i64 -> none , from : %0 : 
!fir.ref>), (%c2_i64 -> none , from : %1 : 
!fir.ref>), (%c0_i64 -> none , release : %2 : 
!fir.ref>), (%c8_i64 -> none , delete : %3 : 
!fir.ref>), (%c6_i64 -> always , from : %4 : 
!fir.ref>))

Requesting comments and suggestions :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/CMakeLists.txt
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -451,6 +451,23 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32, %map: memref) -> () {
+%c6_i64 = arith.constant 6 : i64
+
+// CHECK: omp.target_data if({{.*}}) device({{.*}}) map({{.*}})
+"omp.target_data"(%if_cond, %device, %c6_i64, %map) ({}) {operand_segment_sizes = array, map_operand_segments = array } : ( i1, si32, i64, memref ) -> ()
+
+// CHECK: omp.target_enter_data map({{.*}})
+"omp.target_enter_data"(%c6_i64, %map) {operand_segment_sizes = array, map_operand_segments = array } : ( i64, memref ) -> ()
+
+// CHECK: omp.target_exit_data nowait map({{.*}})
+"omp.target_exit_data"(%c6_i64, %map) {nowait, operand_segment_sizes = array, map_operand_segments = array } : ( i64, memref ) -> ()
+return
+}
+
+
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
===
--- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -11,6 +11,7 @@
 //===--===//
 
 #include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "mlir/Dialect/Arith/IR/Arith.h"
 #include "mlir/Dialect/LLVMIR/LLVMTypes.h"
 #include "mlir/IR/Attributes.h"
 #include "mlir/IR/DialectImplementation.h"
@@ -23,6 +24,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TypeSwitch.h"
 #include 
+#include 
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
 #include "mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc"
@@ -536,6 +538,74 @@
   return success();
 }
 
+//===--===//
+// Parser, printer and verifier for Target Data
+//===--===//
+static ParseResult parseMapClause(
+OpAsmParser ,
+SmallVector> _operands,
+SmallVector> _operand_types) {
+  StringRef mapTypeMod, mapType;
+  OpAsmParser::UnresolvedOperand arg1, arg2;
+  Type arg2Type;
+  auto parseKeyword = [&]() -> ParseResult {
+if (parser.parseLParen() || parser.parseOperand(arg1) ||
+parser.parseArrow() || parser.parseKeyword() ||
+parser.parseComma() || parser.parseKeyword() ||
+parser.parseColon() || parser.parseOperand(arg2) ||
+parser.parseColon() || parser.parseType(arg2Type) ||
+parser.parseRParen())
+  return failure();
+map_operands.push_back({arg1, arg2});
+map_operand_types.push_back({parser.getBuilder().getI64Type(), arg2Type});
+return success();
+  };
+  if (parser.parseCommaSeparatedList(parseKeyword))
+return failure();
+  return success();
+}
+
+static void printMapClause(OpAsmPrinter , Operation *op,
+   OperandRangeRange map_operands,
+   TypeRangeRange map_operand_types) {
+  for (const auto  : map_operands) {
+auto mapTypeBits = 0x00;
+if (auto constOp =
+mlir::dyn_cast(a.front().getDefiningOp()))
+  mapTypeBits = constOp.getValue()
+.cast()
+.getValue()
+.getSExtValue();
+else if (auto constOp = mlir::dyn_cast(
+ a.front().getDefiningOp()))
+  mapTypeBits = constOp.getValue().dyn_cast().getInt();
+
+std::stringstream typeMod, type;
+if (mapTypeBits & 0x04)
+  typeMod << "always ";
+if (mapTypeBits & 0x400)
+  typeMod << "close ";
+if (mapTypeBits & 0x1000)
+  typeMod << "present ";
+if (typeMod.str().empty())
+  typeMod << "none ";
+
+if (mapTypeBits & 0x01)
+  type << "to ";
+if (mapTypeBits & 0x02)
+

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-05 Thread Akash Banerjee via Phabricator via cfe-commits

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

If people are happy with this structure then I'll work on adding a pretty 
printer for the map clause. Please suggest any other changes that are required.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-12-05 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis updated this revision to Diff 480029.
TIFitis added a comment.
Herald added a subscriber: Moerafaat.

Changed map_operands to VariadicOfVariadic type.

Variadic Enums are not supported, hence I've changed the map type to 
VariadicOfVariadic which allows to store the entire map clause including types 
and modifiers in a single operand.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -451,6 +451,27 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32, %map: memref) -> () {
+
+//CHECK: omp.target_data
+"omp.target_data"(%if_cond, %device, %map) ({
+}) {operand_segment_sizes = array, map_operand_segments = array } : ( i1, si32, memref ) -> ()
+
+// CHECK: omp.target_enter_data
+"omp.target_enter_data"(%if_cond, %device) {nowait, operand_segment_sizes = array, map_operand_segments = array } : ( i1, si32) -> ()
+
+// CHECK: omp.target_exit_data
+"omp.target_exit_data"(%if_cond, %device) {nowait, operand_segment_sizes = array, map_operand_segments = array } : ( i1, si32) -> ()
+
+// CHECK: omp.barrier
+omp.barrier
+
+   return
+}
+
+
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
===
--- mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
+++ mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
@@ -23,6 +23,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/TypeSwitch.h"
 #include 
+#include 
 
 #include "mlir/Dialect/OpenMP/OpenMPOpsDialect.cpp.inc"
 #include "mlir/Dialect/OpenMP/OpenMPOpsEnums.cpp.inc"
Index: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
===
--- mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -785,6 +785,154 @@
   let assemblyFormat = "attr-dict";
 }
 
+//===-===//
+// 2.12.2 target data Construct
+//===-===//
+
+def Target_Data: OpenMP_Op<"target_data", [AttrSizedOperandSegments]>{
+  let summary = "target data construct";
+  let description = [{
+Map variables to a device data environment for the extent of the region.
+
+The omp target data directive maps variables to a device data
+environment, and defines the lexical scope of the data environment
+that is created. The omp target data directive can reduce data copies
+to and from the offloading device when multiple target regions are using
+the same data.
+
+The optional $if_expr parameter specifies a boolean result of a
+conditional check. If this value is 1 or is not provided then the target
+region runs on a device, if it is 0 then the target region is executed
+on the host device.
+
+The optional $device parameter specifies the device number for the target
+region.
+
+The optional $use_device_ptr specifies the device pointers to the
+corresponding list items in the device data environment.
+
+The optional $use_device_addr specifies the address of the objects in the
+device data enviornment.
+
+The $map_operands specifies operands in map clause along with it's type and modifiers.
+
+The $map_operand_segments specifies the segment sizes for $map_operands.
+
+TODO:  depend clause and map_type_modifier values iterator and mapper.
+  }];
+
+  let arguments = (ins Optional:$if_expr,
+  Optional:$device,
+  Variadic:$use_device_ptr,
+  Variadic:$use_device_addr,
+  VariadicOfVariadic:$map_operands,
+  DenseI32ArrayAttr:$map_operand_segments);
+
+  let regions = (region AnyRegion:$region);
+
+  let assemblyFormat = [{
+(`if` `(` $if_expr^ `)` )?
+(`device` `(` $device^ `:` type($device) `)` )?
+(`use_device_ptr` `(` $use_device_ptr^ `:` type($use_device_ptr) `)` )?
+(`use_device_addr` `(` $use_device_addr^ `:` type($use_device_addr) `)` )?
+(`map` `(` $map_operands^ `:` type($map_operands) `)` )?
+$region attr-dict
+  }];
+
+}
+
+//===-===//
+// 2.12.3 target enter data Construct
+//===-===//
+
+def Target_EnterDataOp: OpenMP_Op<"target_enter_data",
+

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-10-27 Thread Valentin Clement via Phabricator via cfe-commits

clementval added inline comments.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:828
+(`use_device_addr` `(` $use_device_addr^ `:` type($use_device_addr) `)` )?
+(`map` `(` $map_type_modifier_val `,` $map_type_val `:` $map_operands^ `:` 
type($map_operands) `)` )?
+$region attr-dict

It's not clear how to you deal with multiple map clauses here? Or do you plan 
to have an operation generation per map clause coming from the frontend?



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:879
+(`nowait` $nowait^ )?
+(`map` `(` $map_type_modifier_val `,` $map_type_val `:` $map_operands^ `:` 
type($map_operands) `)` )?
+attr-dict

Same comment here about multiple map clauses. You probably need a custom 
parser/printer for the map part together with the change requested by @TIFitis. 





Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:930
+(`nowait` $nowait^ )?
+(`map` `(` $map_type_modifier_val `,` $map_type_val `:` $map_operands^ `:` 
type($map_operands) `)` )?
+attr-dict

Same comment here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-10-25 Thread Akash Banerjee via Phabricator via cfe-commits

TIFitis requested changes to this revision.
TIFitis added inline comments.
This revision now requires changes to proceed.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:817-818
+ Variadic:$use_device_addr,
+ MapTypeModifierAttr:$map_type_modifier_val,
+ MapTypeAttr:$map_type_val,
+ Variadic:$map_operands);

I think these should be Variadic to allow multiple map clauses.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:871-872
+ UnitAttr:$nowait,
+ MapTypeModifierAttr:$map_type_modifier_val,
+ MapTypeAttr:$map_type_val,
+ Variadic:$map_operands);

I think these should be Variadic to allow multiple map clauses.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:922-923
+ UnitAttr:$nowait,
+ MapTypeModifierAttr:$map_type_modifier_val,
+ MapTypeAttr:$map_type_val,
+ Variadic:$map_operands);

I think these should be Variadic to allow multiple map clauses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-10-19 Thread Raghu via Phabricator via cfe-commits

raghavendhra added a comment.

Ping for review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

2022-10-18 Thread Raghu via Phabricator via cfe-commits

raghavendhra updated this revision to Diff 468448.
raghavendhra added a comment.
Herald added projects: clang, LLVM, Flang.
Herald added subscribers: llvm-commits, cfe-commits.

Seperated map_operands with map-type and map-type-modifier as enum attributes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

Files:
  clang/lib/CodeGen/CGStmtOpenMP.cpp
  flang/lib/Semantics/check-omp-structure.cpp
  llvm/include/llvm/Frontend/OpenMP/OMP.td
  mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
  mlir/test/Dialect/OpenMP/ops.mlir

Index: mlir/test/Dialect/OpenMP/ops.mlir
===
--- mlir/test/Dialect/OpenMP/ops.mlir
+++ mlir/test/Dialect/OpenMP/ops.mlir
@@ -400,6 +400,26 @@
 return
 }
 
+// CHECK-LABEL: omp_target_data
+func.func @omp_target_data (%if_cond : i1, %device : si32,  %data1: memref, %data2: memref, %data3: memref) -> () {
+
+// CHECK: omp.target_data if(%arg0) device(%arg1 : si32) use_device_ptr(%arg2 : memref) use_device_addr(%arg3 : memref) map( present, tofrom : %arg4 : memref) {
+"omp.target_data"(%if_cond, %device, %data1, %data2, %data3) ({
+   // CHECK: omp.terminator
+   omp.terminator
+}) {nowait, operand_segment_sizes = array, map_type_modifier_val = #omp, map_type_val = #omp} : ( i1, si32, memref, memref, memref ) -> ()
+
+// CHECK: omp.target_enter_data if(%arg0) device(%arg1 : si32) nowait map( always,  to : %arg2 : memref)
+"omp.target_enter_data"(%if_cond, %device, %data1)  {nowait, operand_segment_sizes = array, map_type_modifier_val = #omp, map_type_val = #omp} : ( i1, si32, memref ) -> ()
+   
+// CHECK: omp.target_exit_data if(%arg0) device(%arg1 : si32) nowait map( close,  from : %arg2 : memref)
+"omp.target_exit_data"(%if_cond, %device, %data1)  {nowait, operand_segment_sizes = array, map_type_modifier_val = #omp, map_type_val = #omp} : ( i1, si32, memref ) -> ()
+
+   return
+}
+
+
+
 // CHECK-LABEL: omp_target_pretty
 func.func @omp_target_pretty(%if_cond : i1, %device : si32,  %num_threads : i32) -> () {
 // CHECK: omp.target if({{.*}}) device({{.*}})
Index: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
===
--- mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
+++ mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
@@ -770,6 +770,169 @@
   let assemblyFormat = "attr-dict";
 }
 
+//===-===//
+// 2.12.2 target data Construct 
+//===-===//
+
+def Target_Data: OpenMP_Op<"target_data", [AttrSizedOperandSegments]>{
+  let summary = "target data construct";
+  let description = [{
+Map variables to a device data environment for the extent of the region.
+
+The omp target data directive maps variables to a device data 
+environment, and defines the lexical scope of the data environment 
+that is created. The omp target data directive can reduce data copies 
+to and from the offloading device when multiple target regions are using 
+the same data.
+
+The optional $if_expr parameter specifies a boolean result of a
+conditional check. If this value is 1 or is not provided then the target
+region runs on a device, if it is 0 then the target region is executed 
+on the host device.
+
+The optional $device parameter specifies the device number for the target 
+region.
+
+The optional $use_device_ptr specifies the device pointers to the 
+corresponding list items in the device data environment.
+
+The optional $use_device_addr specifies the address of the objects in the 
+device data enviornment.
+
+The $map_operands specifies operands in map clause.
+
+The $map_type specifies map-type within map clause and can take values to,
+from, tofrom or alloc.
+
+The $map_type_modifier specifies the modifier and can be always, close,
+present, iterator and mapper.
+
+TODO:  depend clause and map_type_modifier values iterator and mapper.
+  }];
+
+  let arguments = (ins Optional:$if_expr,
+ Optional:$device,
+ Variadic:$use_device_ptr,
+ Variadic:$use_device_addr,
+ MapTypeModifierAttr:$map_type_modifier_val,
+ MapTypeAttr:$map_type_val,
+ Variadic:$map_operands);
+   
+  let regions = (region AnyRegion:$region); 
+  
+  let assemblyFormat = [{
+(`if` `(` $if_expr^ `)` )?
+(`device` `(` $device^ `:` type($device) `)` )?
+(`use_device_ptr` `(` $use_device_ptr^ `:` type($use_device_ptr) `)` )?
+(`use_device_addr` `(` $use_device_addr^ `:` type($use_device_addr) `)` )?
+(`map` `(` $map_type_modifier_val `,` $map_type_val `:` $map_operands^ `:` type($map_operands) `)` )?
+$region attr-dict
+  }];  
+  
+}
+

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

25 matches

Site Navigation

Mail list logo

Footer information