Skip to content

Lower IO input with vector subscripts #1057

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 17, 2021
Merged

Lower IO input with vector subscripts #1057

merged 1 commit into from
Sep 17, 2021

Conversation

jeanPerier
Copy link
Collaborator

Implements #952.

Define a VectorSubscriptBox class that allow representing and working
with a lowered Designator containing vector subscripts while ensuring
all the subscripts expression are only lowered once.

It's a bit of a super ExtendedValue (but is is not added as such because
it is heavy and its use case is only restricted to IO input so far).

The key point of this class is that it has members functions
loopOverElements and loopOverElementsWhile that allow creating loops
over each element of the designator, and call a provided callback with
the address of the element.

This class is used in input IO to create an IO runtime call for each
element of the designator, since it is not possible to build a
descriptor for it. The loopOverElementsWhile version is required when
error recovery is enabled in IO.

A hidden VectorSubscriptBoxBuilder is in charge of making a custom
Designator visit and create the VectorSubscriptBox. Once this is
done, the VectorSubscriptBox does not equire any front-end data
structures to work with.

The motivation for creating such tool is that the current lowering
array lowering infrastructure is centered around assignment (to a
variable or temporary), and could not be used here to created the loops
over the IO runtime calls without some non trivial modification to it.
Adding complexity to the array expression lowering framework to cover
a corner case did not appear a good idea.

The option of creating a temp, passing it to the runtime, and copying it
back was explored, but it was not possible to guarantee that the
subscript would be evaluated only once given there was no way to "keep
the lowered representation" of the designator between the temp creation
and the copy back (the temp creation requires evaluating the susbcripts
to compute the shape in general). However, note that with the added
VectorSubscriptBox, it would actually now also be possible to deploy a
temp + copy back mechanism. This can be done later if it appeared
beneficial in real world program.

The only cases left TODOs are the cases when one of the Components
is a parent type reference (not yet handled properly in the general case),
the coarray case, and the PDT case (not sure exactly how type params
will be threaded in fir.field_index). All other cases are covered, and
I tried to add exhaustive tests since I do not expect real world program
to be very harsh on this utility (most will just do READ(*, *) x(y)).

Some helpers are moved from ConvertExpr so they can be used in the newly added file (ignorEvConvert, arraySectionElementToExtendedValue).

Copy link
Collaborator

@mleair mleair left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@psteinfeld psteinfeld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like some files need to have clang-format run on them and the run of check-flang needs to be fixed.

Copy link
Collaborator Author

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks to me like some files need to have clang-format run on them and the run of check-flang needs to be fixed.

Thanks for catching this, should be fixed by ccccb8f055f42aa4dc3a3204638c13a252dc3beb.

@psteinfeld
Copy link
Collaborator

Looks great! Thanks for fixing this up.

Copy link

@schweitzpgi schweitzpgi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for providing a place to tidy up some of the component and path lowering.

mustBox ? builder.createBox(loc, element) : element);
};
if (!ok)
ok = builder.createBool(loc, true);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it is really worthwhile to lazily create the true constant... It doesn't hurt anything but it seems to invite bugs of omission.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think checkResult and ok could be merge into a single thing, the difference would be that an if ok/else would be generated for the first io-item, which is not the case today. I won't do it as part of this PR because it require changing more stuff unrelated to vector subscripts in IO (and will probably require some lit test updates). But if it sounds like to you something we should do, I can put a separate PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that might work as well. There is no rush and bugs have higher priority.

Define a VectorSubscriptBox class that allow representing and working
with a lowered Designator containing vector subscripts while ensuring
all the subscripts expression are only lowered once.

It's a bit of a super ExtendedValue (but is is not added as such because
it is heavy and its use case is only restricted to IO input so far).

The key point of this class is that it has members functions
`loopOverElements` and `loopOverElementsWhile` that allow creating loops
over each element of the designator, and call a provided callback with
the address of the element.

This class is used in input IO to create an IO runtime call for each
element of the designator, since it is not possible to build a
descriptor for it. The `loopOverElementsWhile` version is required when
error recovery is enabled in IO.

A hidden VectorSubscriptBoxBuilder is in charge of making a custom
Designator<T> visit and create the VectorSubscriptBox. Once this is
done, the VectorSubscriptBox does not equire any front-end data
structures to work with.

The motivation for creating such tool is that the current lowering
array lowering infrastructure is centered around assignment (to a
variable or temporary), and could not be used here to created the loops
over the IO runtime calls without some non trivial modification to it.
Adding complexity to the array expression lowering framework to cover
a corner case did not appear a good idea.

The option of creating a temp, passing it to the runtime, and copying it
back was explored, but it was not possible to guarantee that the
subscript would be evaluated only once given there was no way to "keep
the lowered representation" of the designator between the temp creation
and the copy back (the temp creation requires evaluating the susbcripts
to compute the shape in general). However, note that with the added
VectorSubscriptBox, it would actually now also be possible to deploy a
temp + copy back mechanism. This can be done later if it appeared
beneficial in real world program.

The only cases left TODOs are the cases when one of the Components
is a parent type reference (not yet handled properly in the general case),
the coarray case, and the PDT case (not sure exactly how type params
will be threaded in fir.field_index). All other cases are covered, and
I tried to add exhaustive tests since I do not expect real world program
to be very harsh on this utility (most will just do READ(*, *) x(y)).
@jeanPerier jeanPerier merged commit 40d96d4 into fir-dev Sep 17, 2021
@jeanPerier jeanPerier deleted the jpr-vector-io-2 branch September 17, 2021 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants