This is an archive of the discontinued LLVM Phabricator instance.

[flang] Make 'team_number()' an intrinsic function
ClosedPublic

Authored by craig.rasmussen on Apr 20 2021, 3:27 PM.

Details

Summary

Added 'team_number()' to the list of functions that are evaluated as intrinsic. Also changed the type of TEAM_TYPE (in TypePattern) to be DerivedType rather than IntType.

Implementation now no longer needs the static function MissingDerivedType() hack.

Diff Detail

Event Timeline

craig.rasmussen requested review of this revision.Apr 20 2021, 3:27 PM
craig.rasmussen created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptApr 20 2021, 3:27 PM
flang/lib/Evaluate/intrinsics.cpp
107

I think that the type here should be DerivedType rather than IntType (as I've changed it to be). However, I'm not sure as I'm rather confused about the implications of this. It does apparently fix the intrinsic function image_status() (for which there isn't a test yet).

747

I tried to create only one entry here for team_number but couldn't work it out. It seems like Optionality::optional should work but it failed for me (see RequiredTeam and associated comments in the diff above).

1297

It seems like more work is required here because argOk==true should only occur if the derived type is type(team_type), right? Can derived types have kinds (I suppose I should know that)?

In any case, once TEAM_TYPE is sorted out I can finish a lot more of coarray intrinsic functions.

klausler added inline comments.Apr 21 2021, 8:37 AM
flang/lib/Evaluate/intrinsics.cpp
236

Why not use it?

1297

Right, you need to check that the derived type is TEAM_TYPE from the intrinsic module ISO_FORTRAN_ENV. Grep the sources and you'll find a utility predicate that should help.

  1. Fixed team argument in call to team_number so that it is optional.
  2. Fixed teamType kind in flang/lib/Evaluate/intrinsics.cpp
  3. Added team_number test to flang/unittests/Evaluate/intrinsics.cpp
flang/include/flang/Common/default-kinds.h
59 ↗(On Diff #341250)

This was a guess.

flang/unittests/Evaluate/intrinsics.cpp
159 ↗(On Diff #341250)

Team is for using a team actual argument in TestCall below at line 323, I don't know how to work this out.

321 ↗(On Diff #341250)

Should be fixed or removed I assume.

klausler requested changes to this revision.Apr 28 2021, 10:21 AM
klausler added inline comments.
flang/include/flang/Common/default-kinds.h
59 ↗(On Diff #341250)

The kind type parameters of derived types are entirely under the control of the program(mer); there can be an arbitrary number of kind type parameters, including zero, and there are no meaningful defaults apart from what a given type's definition may specify.

flang/include/flang/Evaluate/type.h
73 ↗(On Diff #341250)

Derived types are not intrinsic types; this distinction is their reason for being. Please don't break this predicate.

flang/lib/Common/default-kinds.cpp
79 ↗(On Diff #341250)

See above. There can be no meaningful default kind that applies to all derived types.

This revision now requires changes to proceed.Apr 28 2021, 10:21 AM

I forgot to add that there is one test failing at the moment which makes no sense to me.

FAIL: flang-OldUnit :: Evaluate/folding.test (714 of 799)

  • TEST 'flang-OldUnit :: Evaluate/folding.test' FAILED ****

/Users/rasmussen17/Compilers/llvm-project/flang/unittests/Evaluate/folding.cpp:64: FAIL: y1Flushing.IsInfinite() || std::abs(host::CastFortranToHost<R4>(y1Flushing) + 88.) > 2
73 tests pass, 1 test FAILS



Failed Tests (1):

flang-OldUnit :: Evaluate/folding.test

I forgot to add that there is one test failing at the moment which makes no sense to me.

FAIL: flang-OldUnit :: Evaluate/folding.test (714 of 799)

I assume you're running on a mac. I've seen that too but haven't been able to figure out what the problem is. It's not related to your change so you can ignore it.

Added the static function MissingDerivedType() to allow the creation of a DynamicType with TypeCategory::Derived when a semantics::DerivedTypeSpec is not directly available. This allows a missing actual argument for an optional dummy.

Added line comments.

flang/include/flang/Evaluate/type.h
132 ↗(On Diff #357042)

This function (whatever it is named) is critical to get team_number to work with a missing optional team arguments. Otherwise the check

CHECK(IsValidKindOfIntrinsicType(category_, kind_));

disallows the construction of a DynamicType that is not an intrinsic type.

226 ↗(On Diff #357042)

An special enum value is not really needed (I'm pretty sure) as a kind value of 0 could be used. Having a special kind value could be useful for later analysis but no usage identified at the moment.

flang/lib/Evaluate/intrinsics.cpp
1675

Since there is no actual argument at this point a semantics::DerivedTypeSpec can't be created from it. Nor does it seem possible to create a DerivedTypeSpec from the context as it is a FoldingContext not a semantics::SemanticsContext.

klausler added inline comments.Jul 7 2021, 12:54 PM
flang/include/flang/Evaluate/type.h
132 ↗(On Diff #357042)

When the derived type TEAM_TYPE from intrinsic module ISO_FORTRAN_ENV is available, this MissingDerivedType() is not required, right?

craig.rasmussen added inline comments.Jul 7 2021, 1:26 PM
flang/include/flang/Evaluate/type.h
132 ↗(On Diff #357042)

Correct. This is tested by the new test team_number.f90 (added in this patch). If the actual argument is available the dummy argument is created from the actual with

auto dc{characteristics::DummyArgument::FromActual(
    std::string{d.keyword}, *expr, context)};

see semantics.cpp line 1651. Although to be clear, the derived type TEAM_TYPE will have already been loaded from ISO_FORTRAN_ENV via a USE statement by this time, it is just not directly available.

klausler added inline comments.Jul 7 2021, 1:43 PM
flang/include/flang/Evaluate/type.h
132 ↗(On Diff #357042)

So when the optional argument is absent, the type is *not* TEAM_TYPE from ISO_FORTRAN_ENV, even if that type is available?

flang/include/flang/Evaluate/type.h
132 ↗(On Diff #357042)

I'm not sure sure I understand the question fully but I'll give it a try. If the actual argument is missing, the dummy argument type, TEAM_TYPE, remains the same. However, one can't create a characteristics::DummyArgument from the actual argument (which happens if the actual is present) as it is missing. Thus I created the new function MissingDerivedType() to create a DynamicType.

Admittedly, the DynamicType is not well formed because the derived_ member is a nullptr. However, is the semantic checking that follows this does not create a problem because when the actual argument is missing, there is not type comparison done between the actual argument (since it is missing) and the dummy argument.

If I understand correctly, I don't think the AST is mutated during this pass the DynamicType returned by MissingDerivedType() is not ultimately kept around.

craig.rasmussen edited the summary of this revision. (Show Details)
  1. Added "! REQUIRES: shell" to test file so as not to trip on windows
  2. Ran git clang-format

@klausler, are any further changes required to get this approved?

@klausler, are any further changes required to get this approved?

Have you given up trying to construct a valid TEAM_TYPE derived type to characterize an absent TEAM= actual argument?

In a response @klausler asked:

Have you given up trying to construct a valid TEAM_TYPE derived type to characterize an absent TEAM= actual argument?

Yes. When I looked into doing this it didn't seem possible without major changes to the function interfaces in lib/Evaluate/intrinsics.cpp. As far as I can tell, it doesn't seem possible to create a DerivedTypeSpec from the context, as it is a FoldingContext not a semantics::SemanticsContext.

In a response @klausler asked:

Have you given up trying to construct a valid TEAM_TYPE derived type to characterize an absent TEAM= actual argument?

Yes. When I looked into doing this it didn't seem possible without major changes to the function interfaces in lib/Evaluate/intrinsics.cpp. As far as I can tell, it doesn't seem possible to create a DerivedTypeSpec from the context, as it is a FoldingContext not a semantics::SemanticsContext.

I'll have to do it, then.

In a response @klausler asked:

Have you given up trying to construct a valid TEAM_TYPE derived type to characterize an absent TEAM= actual argument?

Yes. When I looked into doing this it didn't seem possible without major changes to the function interfaces in lib/Evaluate/intrinsics.cpp. As far as I can tell, it doesn't seem possible to create a DerivedTypeSpec from the context, as it is a FoldingContext not a semantics::SemanticsContext.

I'll have to do it, then.

Would it be possible to create a constant of type TEAM_TYPE in iso_fortran_env.f90 or __fortran_builtins.f90 to indicate the current team and then use that constant as the value of the missing optional argument? This would mean that iso_fortran_env.f90 would need to be available to the compiler to process calls to the TEAM_NUMBER() intrinsic.

Would it be possible to create a constant of type TEAM_TYPE in iso_fortran_env.f90 or __fortran_builtins.f90 to indicate the current team and then use that constant as the value of the missing optional argument? This would mean that iso_fortran_env.f90 would need to be available to the compiler to process calls to the TEAM_NUMBER() intrinsic.

Sure, but finding that constant is no easier than finding TEAM_TYPE itself. Not a big deal, and I'll look into it when I'm out from under the f95 semantics items.

MehdiChinoune added a project: Restricted Project.Sep 15 2021, 11:49 AM

Implemented TEAM_NUMBER intrinsic function without use of MissingDerivedType() hack.

craig.rasmussen edited the summary of this revision. (Show Details)Sep 20 2021, 1:34 PM
craig.rasmussen marked an inline comment as done.Sep 27 2021, 2:06 PM
craig.rasmussen added inline comments.
flang/lib/Evaluate/intrinsics.cpp
1297

I had found semantics::IsTeamType which worked fine when static libraries are built, however fails when building shared libraries. Spent too much time trying to figure out a way around the linker but its pretty clear that the design doesn't really allow this because of library build order. Now just using a simple lookup for symbol name "__builtin_team_type". Will submit new patch shortly after more testing,

klausler added inline comments.Sep 27 2021, 2:09 PM
flang/lib/Evaluate/intrinsics.cpp
1297

Inlined implementation of call to semantics::IsTeamType. This is necessary because the semantics::IsTeamType symbol in undefined at the time libFortranEvalute is built.

flang/lib/Evaluate/intrinsics.cpp
1297

Ok, just noticed your comment. I'll update the patch.

craig.rasmussen added a comment.EditedSep 27 2021, 4:48 PM

This needs to be updated for changes in https://reviews.llvm.org/D110356

Update based on changes in https://reviews.llvm.org/D110356. Cleaned up tests.

klausler accepted this revision.Sep 30 2021, 10:22 AM
This revision is now accepted and ready to land.Sep 30 2021, 10:22 AM
This revision was landed with ongoing or failed builds.Oct 4 2021, 12:39 PM
This revision was automatically updated to reflect the committed changes.

Looks like buildbots are failing with this revision. Can you have a look or revert?

The failure we have is: https://lab.llvm.org/buildbot/#/builders/176/builds/951/steps/12/logs/FAIL__flang-OldUnit__intrinsics_test

INTERNAL: The __fortran_builtins module was not found, and the type '__builtin_team_type' was required.

Is isn't, https://lab.llvm.org/buildbot/#/builders/177/builds/1621.

False alarm then, thanks! I wondered why the flang bots were being very slow today, turns out they aren't at all.