[Mlir-commits] [mlir] [OpenMP][MLIR] Add `private` clause to `omp.target` (PR #91202)

[Sergio Afonso]

================
@@ -1048,6 +1052,48 @@ static void printMapEntries(OpAsmPrinter &p, Operation *op,
   }
 }
 
+static ParseResult parsePrivateList(
+    OpAsmParser &parser,
+    SmallVectorImpl<OpAsmParser::UnresolvedOperand> &privateOperands,
+    SmallVectorImpl<Type> &privateOperandTypes, ArrayAttr &privatizerSymbols) {
+  SmallVector<SymbolRefAttr> privateSymRefs;
+  SmallVector<OpAsmParser::Argument> regionPrivateArgs;
+
+  if (failed(parser.parseCommaSeparatedList([&]() {
+        if (parser.parseAttribute(privateSymRefs.emplace_back()) ||
+            parser.parseOperand(privateOperands.emplace_back()) ||
+            parser.parseArrow() ||
+            parser.parseArgument(regionPrivateArgs.emplace_back()) ||
+            parser.parseColonType(privateOperandTypes.emplace_back()))
+          return failure();
+        return success();
+      })))
+    return failure();
+
+  SmallVector<Attribute> privateSymAttrs(privateSymRefs.begin(),
+                                         privateSymRefs.end());
+  privatizerSymbols = ArrayAttr::get(parser.getContext(), privateSymAttrs);
+
+  return success();
+}
+
+static void printPrivateList(OpAsmPrinter &p, Operation *op,
+                             ValueRange privateVarOperands,
+                             TypeRange privateVarTypes,
+                             ArrayAttr privatizerSymbols) {
+  auto targetOp = mlir::dyn_cast<mlir::omp::TargetOp>(op);
+  assert(targetOp);
+
+  auto &region = op->getRegion(0);
+  auto *argsBegin = region.front().getArguments().begin();
+  MutableArrayRef argsSubrange(argsBegin + targetOp.getMapOperands().size(),
----------------
skatrak wrote:

It looks like we should have some common method to query OpenMP operations to return the index where certain groups of block arguments start. Because having operation-specific logic here doesn't look very scalable. I'm thinking something like this perhaps:
```c++
auto iface = cast<OpenMPOpWithEntryBlockArgs>(op);
int startIdx = iface.getPrivateArgsIndex();
if (startIdx < 0) { /* This instance doesn't have any privatization-related block arguments */ }
MutableArrayRef<BlockArgument> privateArgs(argsBegin + startIdx, argsBegin + startIdx + privateVarTypes.size());
...
```
That interface should be extensible to also allow queries for `getReductionArgsIndex()` and possibly others in the future.

Thinking about it a bit more, maybe the `get<Clause>BlockArgsIndex()` and `get<Clause>BlockArgsSize()` functions would be part of the ops themselves and the interface could define `get<Clause>Args()` functions to return references to the entry block arguments, based on calls to these two. Any operation implementing the interface must also implement these functions.
```c++
auto iface = cast<OpenMPOpWithEntryBlockArgs>(op);
ArrayRef<BlockArgument> privateArgs = iface.getPrivateArgs();
...
```

https://github.com/llvm/llvm-project/pull/91202