This is an archive of the discontinued LLVM Phabricator instance.

[flang] Add runtime support for GET_COMMAND
ClosedPublic

Authored by rovka on Feb 2 2022, 4:24 AM.

Details

Summary

Implement the GET_COMMAND intrinsic.
Add 2 new parameters (sourceFile and line) so we can create
a terminator for RUNTIME_CHECKs.

Diff Detail

Event Timeline

rovka created this revision.Feb 2 2022, 4:24 AM
rovka requested review of this revision.Feb 2 2022, 4:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 4:24 AM

The implementation looks good to me, thanks. I have the same comment as @klausler in D118776. We recently realized that lowering was not dealing properly with argument that may be dynamically optional:

subroutine foo(command, length, errmsg, status)
  character(*), optional :: command, length, errmsg
  integer, optional :: status
  call get_command(command, length, errmsg, status)
end subroutine

It is not until the runtime that we will know which arguments are present here. So splitting the intrinsic in different entry points/returning the status by value (which made sense to me before), turns out to push some complexity in lowering here. I am not sure about get_command, but some intrinsics have requirements that at exactly one optional argument be present (RANDOM_SEED), and it is possible but cumbersome to deal with checking these constraints at runtime in inlined code. Having split entry points prevents the runtime from being able to check this. Since these intrinsics are not "hot code", I think moving to a single entry points approach and let the runtime check optionality constraints and dispatching makes more sense.

flang/runtime/command.cpp
166

should this be max(0, executionEnvironment.argc - 1) ? I think it is highly unlikely that argc be zero, but looking at some stack overflow threads, that does not look impossible.

rovka updated this revision to Diff 411088.Feb 24 2022, 5:08 AM
rovka edited the summary of this revision. (Show Details)

I finally got around to updating this to write the length to an int descriptor.
Also:
Added the source & line parameters so we can have RUNTIME_CHECKS.
Added a test fixture with a completely empty argv.

I'm going to update the other intrinsics in the file to the same model (i.e. single entry point with int descriptors, RUNTIME_CHECKs for invalid descriptors, support for empty argv), and with that I can probably remove some redundant checks from some of the helpers. That's going to be a different patch though.

jeanPerier added inline comments.Mar 2 2022, 4:52 AM
flang/runtime/command.cpp
174

Could this be moved right after stat = CopyToDescriptor(*value, " ", 1, errmsg, offset); on line 184 ?
It seems related to it but having it after the if {} where the copy happens makes it harder for me to be sure this is related.

flang/unittests/Runtime/CommandTest.cpp
358

This test seems to fail on windows, see the build bot:

Note: Google Test filter = OnlyValidArguments.GetCommandShortLength
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from OnlyValidArguments
[ RUN      ] OnlyValidArguments.GetCommandShortLength
C:\ws\w2\llvm-project\premerge-checks\flang\unittests\Runtime\CommandTest.cpp(77): error: Expected equality of these values:
  *value->OffsetElement<std::int64_t>()
    Which is: 8589934643
  expected
    Which is: 51

I think the value->OffsetElement<std::int64_t>() in CheckDescriptorEqInt may be undefined behavior if the integer type in value is not an std::int64_t (like here where it is a short).

Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2022, 4:52 AM
rovka added inline comments.Mar 3 2022, 11:03 PM
flang/unittests/Runtime/CommandTest.cpp
358

Oops, I thought I had looked at the premerge checks for this one, but it must've slipped my mind 😳 Thanks for looking into it.

rovka updated this revision to Diff 412938.Mar 3 2022, 11:12 PM
  • Replaced CopyToDescriptor with CheckAndCopyToDescriptor.
  • Made DescriptorEqInt a template, so we can check with the intended integer type.
jeanPerier accepted this revision.Mar 10 2022, 5:32 AM

Looks good, thanks

This revision is now accepted and ready to land.Mar 10 2022, 5:32 AM
This revision was automatically updated to reflect the committed changes.
This comment was removed by jeanPerier.