This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP][WIP] Separate clause translation from directive translation
AbandonedPublic

Authored by shraiysh on Mar 14 2022, 2:50 AM.

Details

Summary

This patch separates clause translation from direction translation while converting F90 to OpenMP Dialect. This clause handler can be reused for other operations like wsloop, sections etc.

Diff Detail

Event Timeline

shraiysh created this revision.Mar 14 2022, 2:50 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
shraiysh requested review of this revision.Mar 14 2022, 2:50 AM
clementval added inline comments.Mar 15 2022, 8:33 AM
flang/lib/Lower/OpenMP.cpp
67

The name is a bit misleading. Something with OpenMP in the name would make more sense IMO.

shraiysh updated this revision to Diff 416169.Mar 17 2022, 7:05 AM

Thanks for the review @clementval. Addressed comment.

peixin added inline comments.Mar 23 2022, 7:48 AM
flang/lib/Lower/OpenMP.cpp
67

How about removing MLIR? OpenMPClauses or OmpClauses?

71

We used someClauseOperand(s) for other constructs in OpenMP.cpp in fir-dev. Can we keep consistent with others?

74

handleProcBindClause -> genProcBindClause to be consistent with other function names in this file.

Thanks @peixin for the comments. I will rework on this once upstreaming of translations has been done. My plan is to remove individual clause handling from genOMP functions and put it in a collect() function so all the constructs can use it. I will ping this thread once the upstreaming is done.

peixin added inline comments.Apr 2 2022, 2:38 AM
flang/test/Lower/OpenMP/parallel.f90
2

I am thinking if flang_fc1 -fopenmp -emit-fir should be added. Check the comment in https://reviews.llvm.org/D122595. Also you can check the discussions in https://reviews.llvm.org/D121171#3370137. If new passes is added in flang-new, I am not sure if it will crash the test cases.

16

Please add CHECK-LABEL for each subroutine.

shraiysh updated this revision to Diff 424102.Apr 20 2022, 11:26 PM

Added handling for block constructs, sections constructs and critical construct.

shraiysh retitled this revision from [flang][OpenMP] Lowering clauses for parallel construct to [flang][OpenMP][WIP] Separate clause translation from directive translation.Apr 20 2022, 11:28 PM
shraiysh edited the summary of this revision. (Show Details)
peixin added inline comments.Apr 24 2022, 8:09 PM
flang/lib/Lower/OpenMP.cpp
144

As I mentioned in D124138, there is one drawback collecting clauses in one place for all block constructs. For those unsupported clauses, one hard TODO is changed into one unknown result, maybe one segfault. It is easy to omit some features support and get users into trouble if there is segfault/ICE.

Can we let each construct have their own collect? Here is my recomendation:

struct OpenMPMLIRClauses {
    void genNumThreadsClause(
      const Fortran::parser::OmpClause &clause,
      mlir::Value &mlirNumThreadsExpr)
     void genIFClause(...)
  ...
}
if (blockDirective.v == llvm::omp::OMPD_parallel) {
  for (const Fortran::parser::OmpClause &clause : clauseList.v) {
    genNumThreadsClause(...); ! if lowering to LLVM IR has been supported.
    genIfClause(...); ! if lowering to LLVM IR has been supported.
    TODO(... clause unsupported for parallel construct);
  }
  ...
} else if (blockDirective.v == llvm::omp::OMPD_master) {

As we pursue the OpenMP standard, it is easily know what are not supported yet. And this refector patch won't need to wait for other patches.
@shraiysh @kiranchandramohan @clementval What do you think?

@shraiysh Can you restart this work? There are two small patches left to upstream and it won't be affected too much.

To refactor the code, there are several things that I can think of to handle:

  1. Explicit clauses for each construct. One unsupported clause should be given one TODO.
  2. The clauses which are not allowed in directives should be given one assert?
  3. The combined constructs are not carefully organized.
flang/lib/Lower/OpenMP.cpp
144

I take this back. This cause the code duplication.

Sure @peixin, thank you for pinging here. I will restart this work soon and try to incorporate your ideas and concerns.

shraiysh planned changes to this revision.May 30 2022, 5:42 AM

Hi @shraiysh, @peixin. I just realized about this patch, thanks to @kiranchandramohan, after I spent time working on a patch with basically the same goal of refactoring the clause processing in the PFT to MLIR lowering. The other patch is here: D155981.

It follows a slightly different approach where there is no collect() method or state to be kept, so the responsibility of deciding which clauses to generate code for is down to the caller. I am interested to see if you have any comments to improve that approach before landing it. It does other nonfunctional changes apart from extracting the clause-processing logic into a class, so the diff can be a bit noisy.

Thank you for working on this @skatrak. I will abandon this patch now.

shraiysh abandoned this revision.Aug 28 2023, 6:53 AM