This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by TIFitis on Mar 17 2023, 5:45 AM.

Details

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

Diff Detail

Event Timeline

TIFitis created this revision.Mar 17 2023, 5:45 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 17 2023, 5:45 AM
TIFitis requested review of this revision.Mar 17 2023, 5:45 AM
TIFitis updated this revision to Diff 506099.Mar 17 2023, 8:52 AM

Added TODO for symbol lookup failure.

TIFitis updated this revision to Diff 506101.Mar 17 2023, 9:05 AM

Missed if condition in previous entry.

luporl added a subscriber: luporl.Mar 21 2023, 5:52 AM

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 edited the summary of this revision. (Show Details)Mar 21 2023, 7:36 AM
TIFitis added inline comments.Mar 21 2023, 7:40 AM
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?

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 added inline comments.Mar 21 2023, 8:10 AM
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.

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 updated this revision to Diff 508093.Mar 24 2023, 7:37 AM
TIFitis marked 5 inline comments as done.

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.

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 updated this revision to Diff 508570.Mar 27 2023, 4:24 AM
TIFitis marked an inline comment as done.

Addressed reviewer comments.

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.

This revision is now accepted and ready to land.Mar 27 2023, 4:30 AM
This revision was landed with ongoing or failed builds.Mar 27 2023, 4:38 AM
This revision was automatically updated to reflect the committed changes.