Page MenuHomePhabricator

[flang][openacc] Fix reduction operator parsing/unparsing
ClosedPublic

Authored by clementval on Aug 20 2020, 8:27 AM.

Details

Summary

Update the way reduction operator are defined for the OpenACC parser.

Diff Detail

Event Timeline

clementval created this revision.Aug 20 2020, 8:27 AM
Herald added a project: Restricted Project. · View Herald Transcript
clementval requested review of this revision.Aug 20 2020, 8:27 AM

Sorry, I have not gone into detail here. But could you provide some motivation/explanation for this change? Is this because OpenACC accepts fewer or different reduction operators. Or is a similar change required in OpenMP too.

clementval added a comment.EditedAug 20 2020, 9:49 AM

Sorry, I have not gone into detail here. But could you provide some motivation/explanation for this change? Is this because OpenACC accepts fewer or different reduction operators. Or is a similar change required in OpenMP too.

In the current state, only reduction with + and * would be parsed and resolved correctly. We would need an intrinsic function name resolution in place to accept the others operators and also a semantic check to discard all intrinsic that are not accepted as reduction operators. So in order to simply thing, a simple enumeration seemed a better replacement. OpenACC doesn't accept user-defined reduction operator (as of 3.0) so we can make this simplification. For OpenMP, as you can use user-defined operator I guess you will have to put in place a function name resolution for those reduction operators. But in the current state the openmp reduction operator such as max or min are likely to fail at name resolution.

LGTM. But because this is different from OpenMP handling it will be best if @klausler or @sscalpone approves.

Would you have issues matching the operation in Fortran and the reduction operation in the OpenACC directive since you are using your own custom enums?

flang/include/flang/Parser/parse-tree.h
3846

For my information: Why is source needed here?

Would you have issues matching the operation in Fortran and the reduction operation in the OpenACC directive since you are using your own custom enums?

I'm not sure I get what you want to say here.

flang/include/flang/Parser/parse-tree.h
3846

Just to hold the source location information for error reporting. So error can be reported on the reduction operator.

Would you have issues matching the operation in Fortran and the reduction operation in the OpenACC directive since you are using your own custom enums?

I'm not sure I get what you want to say here.

What I meant to say here is that previously the OpenACCReductionOperator was defined in terms of struct DefinedOperator which in turn is defined in terms of DefinedOpName or IntrinsicOperator. If these are also used for the Fortran expressions in the parse tree then it is easy to match the OpenACC Reduction Operation and the operation in Fortran source where reduction is implied. If you use your own custom definitions for operations then there is some code that you have to write to do the matching.

Would you have issues matching the operation in Fortran and the reduction operation in the OpenACC directive since you are using your own custom enums?

I'm not sure I get what you want to say here.

What I meant to say here is that previously the OpenACCReductionOperator was defined in terms of struct DefinedOperator which in turn is defined in terms of DefinedOpName or IntrinsicOperator. If these are also used for the Fortran expressions in the parse tree then it is easy to match the OpenACC Reduction Operation and the operation in Fortran source where reduction is implied. If you use your own custom definitions for operations then there is some code that you have to write to do the matching.

Ok I see now. Yeah you are right in case I need to do a matching (which is still unclear at this stage) there would be some code to be written to do that. But my guess right now is that we can lower this directly to the dialect without the need to match an operation since what we need are the values taking part in the reduction to issue the correct operation.

klausler accepted this revision.Aug 24 2020, 9:25 AM
This revision is now accepted and ready to land.Aug 24 2020, 9:25 AM
This revision was landed with ongoing or failed builds.Aug 24 2020, 11:23 AM
This revision was automatically updated to reflect the committed changes.