[flang-commits] [mlir] [openmp] [flang] [Flang][OpenMP] Initial mapping of Fortran pointers and allocatables for target devices (PR #71766)

[via flang-commits]

================
@@ -1710,30 +1710,76 @@ bool ClauseProcessor::processLink(
 
 static mlir::omp::MapInfoOp
 createMapInfoOp(fir::FirOpBuilder &builder, mlir::Location loc,
-                mlir::Value baseAddr, std::stringstream &name,
-                mlir::SmallVector<mlir::Value> bounds, uint64_t mapType,
-                mlir::omp::VariableCaptureKind mapCaptureType,
-                mlir::Type retTy) {
-  mlir::Value varPtr, varPtrPtr;
-  mlir::TypeAttr varType;
-
+                mlir::Value baseAddr, mlir::Value varPtrPtr, std::string name,
+                mlir::SmallVector<mlir::Value> bounds,
+                mlir::SmallVector<mlir::Value> members, uint64_t mapType,
+                mlir::omp::VariableCaptureKind mapCaptureType, mlir::Type retTy,
+                bool isVal = false) {
   if (auto boxTy = baseAddr.getType().dyn_cast<fir::BaseBoxType>()) {
     baseAddr = builder.create<fir::BoxAddrOp>(loc, baseAddr);
     retTy = baseAddr.getType();
   }
 
-  varPtr = baseAddr;
-  varType = mlir::TypeAttr::get(
+  mlir::TypeAttr varType = mlir::TypeAttr::get(
       llvm::cast<mlir::omp::PointerLikeType>(retTy).getElementType());
 
   mlir::omp::MapInfoOp op = builder.create<mlir::omp::MapInfoOp>(
-      loc, retTy, varPtr, varType, varPtrPtr, bounds,
+      loc, retTy, baseAddr, varType, varPtrPtr, members, bounds,
       builder.getIntegerAttr(builder.getIntegerType(64, false), mapType),
       builder.getAttr<mlir::omp::VariableCaptureKindAttr>(mapCaptureType),
-      builder.getStringAttr(name.str()));
+      builder.getStringAttr(name));
+
   return op;
 }
 
+static mlir::omp::MapInfoOp processDescriptorTypeMappings(
+    Fortran::semantics::SemanticsContext &semanticsContext,
+    Fortran::lower::StatementContext &stmtCtx,
+    Fortran::lower::AbstractConverter &converter, mlir::Location loc,
+    mlir::Value descriptorAddr, mlir::Value descDataBaseAddr,
+    mlir::ValueRange bounds, std::string asFortran,
+    llvm::omp::OpenMPOffloadMappingFlags mapCaptureType) {
+  llvm::SmallVector<mlir::Value> descriptorBaseAddrMembers;
+  fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
+
+  mlir::Value descriptor = descriptorAddr;
+
+  // The fir::BoxOffsetOp only works with !fir.ref<!fir.box<...>> types, as
+  // allowing it to access non-reference box operations can cause some
+  // problematic SSA IR. However, in the case of assumed shape's the type
+  // is not a !fir.ref, in these cases to retrieve the appropriate
+  // !fir.ref<!fir.box<...>> to access the data we need to map we must
+  // perform an alloca and then store to it and retrieve the data from the new
+  // alloca.
+  if (fir::isAssumedShape(fir::unwrapRefType(descriptorAddr.getType()))) {
+    mlir::OpBuilder::InsertPoint insPt = firOpBuilder.saveInsertionPoint();
+    firOpBuilder.setInsertionPointToStart(firOpBuilder.getAllocaBlock());
+    descriptor =
+        firOpBuilder.create<fir::AllocaOp>(loc, descriptorAddr.getType());
+    firOpBuilder.restoreInsertionPoint(insPt);
+    firOpBuilder.create<fir::StoreOp>(loc, descriptorAddr, descriptor);
+  }
+
+  mlir::Value baseAddrAddr = firOpBuilder.create<fir::BoxOffsetOp>(
+      loc, descriptor, fir::BoxFieldAttr::base_addr);
+
+  descriptorBaseAddrMembers.push_back(createMapInfoOp(
+      firOpBuilder, loc, descDataBaseAddr, baseAddrAddr, asFortran, bounds, {},
+      static_cast<std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
+          mapCaptureType),
+      mlir::omp::VariableCaptureKind::ByRef, descDataBaseAddr.getType()));
+
+  // TODO: map the addendum segment of the descriptor, similarly to the abose
+  // base address/data pointer member.
+
+  return createMapInfoOp(
+      firOpBuilder, loc, descriptor, mlir::Value{}, asFortran, {},
+      descriptorBaseAddrMembers,
+      static_cast<std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
+          mapCaptureType),
+      mlir::omp::VariableCaptureKind::ByRef, descriptor.getType());
----------------
agozillon wrote:

I imagine that it is perfectly fine OpenMP (seems to be to me at least), however, we're missing a chunk of code currently (https://github.com/llvm/llvm-project/blob/main/clang/lib/Sema/SemaOpenMP.cpp#L2140), that will likely have to be introduced as an MLIR opt pass in the future that will categories things as ByRef or ByCopy based on the map variables type and what it's previously been captured in. And at that point it'd likely be a good idea to introduce this case, as currently ByRef covers a reasonable amount of cases, and swapping it to ByCopy appears to make a lot of cases that do pass with ByRef currently fail. 

I believe ByRef/ByCopy is not really a 1:1 for any of the map clauses in OpenMP (although, it can correlate somewhat, and doing the incorrect combination with the incorrect type will anger the libomptarget runtime), it defines how a variable is passed to the device (via the kernel arg structure and reading in from kernel input). For the MLIR -> LLVM IR flow it's still a bit of a WIP and some but not all cases are likely covered, which might be why ByCopy doesn't quite work with all the cases ByRef works with at the moment. 

Take this explanation with a grain of salt, I'm still learning a lot as I go here with the mapping system in Clang and you might know more than me from prior experience! So do feel free to correct me if I am in fact wrong on any point, Johannes, Doru or Alexey are far more knowledgeable on the subject. 

TL;DR: I appreciate this case, and the fact that ByCopy would likely be better for it, but it'd perhaps be better as a future PR rather than in the current PR, where there's a chance to investigate it further.  

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