Page MenuHomePhabricator

[flang] Single entry point for GET_ENVIRONMENT_VARIABLE
ClosedPublic

Authored by luporl on Tue, Jan 24, 9:34 AM.

Details

Summary

This patch refactors the runtime support for GET_ENVIRONMENT_VARIABLE
to have a single entry point instead of 2. It also updates lowering
accordingly.

This makes it easier to handle dynamically optional arguments.
See also https://reviews.llvm.org/D118777

Diff Detail

Event Timeline

luporl created this revision.Tue, Jan 24, 9:34 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptTue, Jan 24, 9:34 AM
luporl published this revision for review.Tue, Jan 24, 9:59 AM

What problem does this patch fix?

AFAIU, having a single entry point for GET_ENVIRONMENT_VARIABLE intrinsic makes it easier to handle dynamically optional arguments correctly.

@jeanPerier explained it in more details in D118777. Lowering was not dealing properly with arguments that may be dynamically optional, that are not known until the runtime.
Splitting the intrinsic in different entry points and returning the length by value pushed some complexity in lowering.

GET_COMMAND_ARGUMENT was already refactored to use a single entry point (D130475).

There is something broken on windows with the unit test changes (I suspect the windows build bot PATH or some other environment variable may be bigger than what the test expects).
This indeed simplifies lowering and the generated code by removing some generated ifs blocks, so I am happy with this change, although I am not sure it changes anything functionally now since lowering eventually dealt with the optional cases "manually".

luporl updated this revision to Diff 492069.Wed, Jan 25, 5:05 AM

Make EnvironmentVariables.Basic test compare only lengths again, to avoid errors with long PATH variables.

jeanPerier accepted this revision.Fri, Jan 27, 12:33 AM

LGTM, thanks

This revision is now accepted and ready to land.Fri, Jan 27, 12:33 AM
This revision was automatically updated to reflect the committed changes.