[Flang][OpenMP] Added TODO checks for unsupported map types

[TIFitis]

Summary

This patch adds TODO checks for unsupported types in the map clause for OpenMP Target directives.

Example of unsupported code:

implicit none
character(len=10) :: str1, str2(5,5)

type t
  character(len=10) :: str1, str2(5,5)
end type t
type(t) :: v

!$omp target enter data map(to: str2(2,5))

!$omp target enter data map(to: v%str1)
!$omp target enter data map(to: v%str2)
!$omp target enter data map(to: v%str2(1,2))

end

[TIFitis]

Added TODO for symbol lookup failure.

[TIFitis]

Missed if condition in previous entry.

[luporl]

Thanks for the patch!

It would be nice if you could add a test or provide a sample code that tries to use an unsupported type.
There is also a typo in the summary (unspported).

flang/lib/Lower/Bridge.cpp

426–429 ↗

(On Diff #506101)

This member function is also used in many other places, that expect the symbol to exist.
So it doesn't seem right to add a TODO here.

Maybe this could be done with a helper function in OpenMP.cpp, that could be used instead of this one, for symbols that may not be supported yet.

Also, the TODO message here doesn't tell what needs to be done.

[TIFitis]

flang/lib/Lower/Bridge.cpp

426–429 ↗

(On Diff #506101)

I agree the TODO here is out of place.

Reason I added it here was that lookupSymbol is private and thus I can't use it from OpenMP.cpp. And I couldn't find anything similar from inside OpenMP.cpp::genObjectList to use which is where the error occurs.

Could you suggest me a way I could check for the unsupported symbol from OpenMP.cpp?

[kiranchandramohan]

You can test the TODOs if you want by adding tests like the following.
https://github.com/llvm/llvm-project/blob/main/flang/test/Lower/OpenMP/Todo/reduction-allocatable.f90

flang/lib/Lower/Bridge.cpp

426–429 ↗

(On Diff #506101)

Could you share the test that causes this issue? Maybe we can look at the type of it and give a TODO error before reaching the lookup?

[TIFitis]

flang/lib/Lower/Bridge.cpp

426–429 ↗

(On Diff #506101)

Yes, I've updated the summary with sample code. Derived types and array sub-sections seem to be the culprit.

[kiranchandramohan]

flang/lib/Lower/OpenMP.cpp

727

You can add some code like the following here and remove the "symbol lookup failed" TODO.

for (const Fortran::parser::OmpObject &ompObject :
     std::get<Fortran::parser::OmpObjectList>(mapClause->v.t).v) {
  if (Fortran::parser::Unwrap<Fortran::parser::ArrayElement>(ompObject) ||
      Fortran::parser::Unwrap<Fortran::parser::StructureComponent>(
          ompObject)) {
    TODO(
        currentLocation,
        "OpenMP TARGET DATA for Array Expressions or Structure Components");
  }
}

745

I think all BoxTypes are not supported. So if a type is a BoxType, you can immediately issue a TODO.

748–751

Will be easier to read if the if statement is written for the unsupported types.

I guess the following type is also not supported.

charTy.getLen() == fir::CharacterType::unknownLen()

[TIFitis]

Addressed reviewer comments.

flang/lib/Lower/OpenMP.cpp

727

I have added this check directly to the genObjectList function so that other users of it can also benefit from the TODO check.
Let me know if you'd want me to check it only for OMP Target Data instead.

748–751

I have added the TODO for all BoxTypes and removed this check.

It seems to catch all the unsupported types other than assumed size arrays but I those are not allowed by specification.

[kiranchandramohan]

flang/lib/Lower/Bridge.cpp

427–428 ↗

(On Diff #508093)

You missed removing this TODO.

flang/lib/Lower/OpenMP.cpp

509–512

This could become a bit confusing as many clauses (like privatisation) do not accept structure components and sometimes array elements as well.

[TIFitis]

Addressed reviewer comments.

[kiranchandramohan]

LGTM. Please see a Nit comment inline.

flang/lib/Lower/OpenMP.cpp

746

Nit: boxType might cause an unused variable warning and error on Werror.

flang/lib/Lower/OpenMP.cpp

if (variable) {
       if (const auto *details =
               sym->detailsIf<Fortran::semantics::HostAssocDetails>()) {
         operands.push_back(converter.getSymbolAddress(details->symbol()));
         converter.copySymbolBinding(details->symbol(), sym);
       }
     }
   };
   for (const Fortran::parser::OmpObject &ompObject : objectList.v) {
     Fortran::semantics::Symbol *sym = getOmpObjectSymbol(ompObject);
     addOperands(*sym);
   }
 }
 
 static mlir::Value
 getIfClauseOperand(Fortran::lower::AbstractConverter &converter,
                    Fortran::lower::StatementContext &stmtCtx,
                    const Fortran::parser::OmpClause::If *ifClause) {
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
   mlir::Location currentLocation = converter.getCurrentLocation();
   auto &expr = std::get<Fortran::parser::ScalarLogicalExpr>(ifClause->v.t);
createTargetDataOp(Fortran::lower::AbstractConverter &converter,
 
   mlir::Value ifClauseOperand, deviceOperand;
   mlir::UnitAttr nowaitAttr;
   llvm::SmallVector<mlir::Value> useDevicePtrOperand, useDeviceAddrOperand,
       mapOperands;
   llvm::SmallVector<mlir::IntegerAttr> mapTypes;
 
   auto addMapClause = [&firOpBuilder, &converter, &mapOperands,
-                       &mapTypes](const auto &mapClause) {
+                       &mapTypes](const auto &mapClause,
+                                  mlir::Location &currentLocation) {
     auto mapType = std::get<Fortran::parser::OmpMapType::Type>(
         std::get<std::optional<Fortran::parser::OmpMapType>>(mapClause->v.t)
             ->t);
     llvm::omp::OpenMPOffloadMappingFlags mapTypeBits =
         llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_NONE;
     switch (mapType) {
     case Fortran::parser::OmpMapType::Type::To:
       mapTypeBits |= llvm::omp::OpenMPOffloadMappingFlags::OMP_MAP_TO;
auto addMapClause = [&firOpBuilder, &converter, &mapOperands,
 
     // TODO: Add support MapTypeModifiers close, mapper, present, iterator
 
     mlir::IntegerAttr mapTypeAttr = firOpBuilder.getIntegerAttr(
         firOpBuilder.getI64Type(),
         static_cast<
             std::underlying_type_t<llvm::omp::OpenMPOffloadMappingFlags>>(
             mapTypeBits));
 
     llvm::SmallVector<mlir::Value> mapOperand;
+    /// Check for unsupported map operand types.
+    for (const Fortran::parser::OmpObject &ompObject :
+         std::get<Fortran::parser::OmpObjectList>(mapClause->v.t).v) {
+      if (Fortran::parser::Unwrap<Fortran::parser::ArrayElement>(ompObject) ||
+          Fortran::parser::Unwrap<Fortran::parser::StructureComponent>(
+              ompObject))
+        TODO(currentLocation,
+             "OMPD_target_data for Array Expressions or Structure Components");
+    }
     genObjectList(std::get<Fortran::parser::OmpObjectList>(mapClause->v.t),
                   converter, mapOperand);
 
     for (mlir::Value mapOp : mapOperand) {
+      /// Check for unsupported map operand types.
+      mlir::Type checkType = mapOp.getType();
+      if (auto refType = checkType.dyn_cast<fir::ReferenceType>())
+        checkType = refType.getElementType();
+      if (checkType.isa<fir::BoxType>())
+        TODO(currentLocation, "OMPD_target_data MapOperand BoxType");
+
       mapOperands.push_back(mapOp);
       mapTypes.push_back(mapTypeAttr);
     }
   };
 
   for (const Fortran::parser::OmpClause &clause : opClauseList.v) {
     mlir::Location currentLocation = converter.genLocation(clause.source);
     if (const auto &ifClause =
             std::get_if<Fortran::parser::OmpClause::If>(&clause.u)) {
       ifClauseOperand = getIfClauseOperand(converter, stmtCtx, ifClause);
     } else if (const auto &deviceClause =
if (const auto &ifClause =
       TODO(currentLocation, "OMPD_target Use Device Ptr");
     } else if (std::get_if<Fortran::parser::OmpClause::UseDeviceAddr>(
                    &clause.u)) {
       TODO(currentLocation, "OMPD_target Use Device Addr");
     } else if (std::get_if<Fortran::parser::OmpClause::Nowait>(&clause.u)) {
       nowaitAttr = firOpBuilder.getUnitAttr();
     } else if (const auto &mapClause =
                    std::get_if<Fortran::parser::OmpClause::Map>(&clause.u)) {
-      addMapClause(mapClause);
+      addMapClause(mapClause, currentLocation);
     } else {
       TODO(currentLocation, "OMPD_target unhandled clause");
     }
   }
 
   llvm::SmallVector<mlir::Attribute> mapTypesAttr(mapTypes.begin(),
                                                   mapTypes.end());
   mlir::ArrayAttr mapTypesArrayAttr =