This is an archive of the discontinued LLVM Phabricator instance.

[NFC][Flang][OpenMP] Refactor OpenMP.cpp::genOpenMPReduction
ClosedPublic

Authored by DylanFleming-arm on Aug 4 2022, 5:46 AM.

Details

Summary

This patch serves two main purposes:
Firstly, to split some of the logic into a seperate method
to try and improve readability

On top of this, it aims to make creating the reductions more generic.
That way, subsequent patches adding reductions shouldn't need
to add a significant amount of extra logic checks, such as checking
for specific operators.

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 4 2022, 5:46 AM
DylanFleming-arm requested review of this revision.Aug 4 2022, 5:46 AM

Makes sense to me, minor suggestions inline.

flang/include/flang/Lower/OpenMP.h
56

Could you add a comment to describe this method? Might be worth including names for the arguments, too. Perhaps this would make sense:

/// Checks whether /p loadVal is is used in an operation the result of which is then stored into /p reductionVal. If yes, then the operation corresponding to the reduction is returned. /p loadVal is assumed to be the value of a load operation and /p reductionVal is the results of an OpenMP reduction operation.
mlir::Operation *getReductionInChain(mlir::Value reductionVal, mlir::Value loadVal);
flang/lib/Lower/OpenMP.cpp
1662

Although the type of loadOperands is an mlir::OpOperand, it's actually something like loadUse (the actual type and what it represents don't match that well). Also, this is definitely singular (rather than plural) as it's just one item at a time :) Same for reductionOperands.

Added comment explaining getReductionInChain, renamed obscure/incorrect variables

awarzynski accepted this revision.Aug 4 2022, 11:30 AM

Thanks for the updates - my remaining comments are nits, so feel free to ignore!

LGTM, though please wait till tomorrow before merging (in case there are more comments).

Also, the failure in the pre-merge CI looks bogus to me, so I would just ignore it. Apparently it's clang-format that failed, but it doesn't say why :) Unless you can see the actual reason for the failure?

flang/lib/Lower/OpenMP.cpp
1660–1664

[nit] Thanks for adding the comment! It should be moved into the header file and written in doxygen format, i.e.:

/// Checks whether /p loadVal is used in an operation, the result of which is then stored into /p reductionVal.
/// If yes, then the operation corresponding to the reduction is returned. 
/// /p loadVal is assumed to be the value of a load operation and /p reductionVal is the results
/// of an OpenMP reduction operation.

I see that you've just followed the style already present in the file. That's also fine and perhaps better for consistency.

1669

[nit] reductionUse would be more consistent with loadUse. And, from what I can tell, equally accurate.

This revision is now accepted and ready to land.Aug 4 2022, 11:30 AM
This revision was landed with ongoing or failed builds.Aug 8 2022, 7:27 AM
This revision was automatically updated to reflect the committed changes.