Page MenuHomePhabricator

[mlir][OpenMP] Add if clause to OpenMP simd construct
ClosedPublic

Authored by domada on Jun 30 2022, 1:05 PM.

Details

Summary

Added if clause support for simd loop construct in definition of OpenMP MLIR dialect.

Diff Detail

Event Timeline

domada created this revision.Jun 30 2022, 1:05 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
domada requested review of this revision.Jun 30 2022, 1:05 PM

Thanks for working on this patch. I think we need to add a testcase in flang/test/Lower/OpenMP/simd.f90

shraiysh added inline comments.Jul 4 2022, 12:30 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
452
  1. Is this different from wsloop control? If not, can we reuse it as LoopControl?
  2. Without a token like for before custom<LoopControl>, the error messages for wrong syntax (like missing loop control) will be very confusing. I would suggest adding for before the custom assembly format to have legible error messages. See omp.wsloop's assemblyFormat for reference.
domada updated this revision to Diff 442026.Jul 4 2022, 1:27 AM

Added test case for simd directive with if clause in file: flang/test/Lower/OpenMP/simd.f90

arnamoy10 added inline comments.Jul 4 2022, 4:55 AM
flang/test/Lower/OpenMP/simd.f90
25

It will be nice to see an example with more realistic looking test case e.g. if (n > threshold), where n and threshold are both arguments, and n is the loop upper bound.

domada updated this revision to Diff 442243.Jul 5 2022, 2:52 AM

Addressed review remarks

domada marked 2 inline comments as done.Jul 5 2022, 2:55 AM
domada added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
452

Ad 1. wsloop control can contain optional inclusive keyword. Is this keyword also allowed for simd loop? I assumed that inclusive keyword is not allowed for simd construct. Moreover, the list of attributes for WsLoop is different than for SIMD loop. That's why I used different method.
Ad 2. Good point -> I will add it.

452

Ad 1. Done -> Added inclusive keyword as well
Ad 2. Done

shraiysh accepted this revision.Jul 5 2022, 3:34 AM

LGTM. Thank you!

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
429

nit: this has to wrap at 80 columns. it looks like it has wrapped much earlier.

This revision is now accepted and ready to land.Jul 5 2022, 3:34 AM
domada updated this revision to Diff 442255.Jul 5 2022, 4:03 AM
domada marked an inline comment as done.

Fixed layout of simd description

domada marked an inline comment as done.Jul 5 2022, 4:05 AM
peixin requested changes to this revision.Jul 5 2022, 4:31 AM

The OMPIRBuilder and lowering to LLVMIR for if clause of SIMD construct is not supported yet. If you merge this before implementing those, users will get unexpected results for a fortran code since it behaves as if there is no if clause.

Usually, we implement the OMPIRBuilder and lowering to LLVMIR first in case some addition process needs to be done in lowering to MLIR. I would recommend you split this patch into two: 1. MLIR operation def (can be merged now) 2. lowering to MLIR (delayed after supporting OMPIRBuilder and lowering to LLVMIR). It's better to verify that the generated MLIR for if clause of SIMD construct in this patch works OK in OMPIRBuilder.

Also, I noticed there should be TODO for other clauses of SIMD construct. I think the lowering of clauses really should be refactored. @shraiysh Do you plan to continue that work? If yes, please add TODO for clauses of SIMD construct.

flang/lib/Lower/OpenMP.cpp
839
This revision now requires changes to proceed.Jul 5 2022, 4:31 AM

@shraiysh Do you plan to continue that work?

Not at the moment no. I am working on other tasks at the moment and might not be able to spend time on it for a couple weeks. If there is an urgent requirement, it's okay if someone else commandeers that revision - D121583.

domada added inline comments.Jul 5 2022, 5:01 AM
flang/lib/Lower/OpenMP.cpp
839

Do you propose to move this if statement inside for loop: for (const auto &clause : loopOpClauseList.v) ?

The OMPIRBuilder and lowering to LLVMIR for if clause of SIMD construct is not supported yet. If you merge this before implementing those, users will get unexpected results for a fortran code since it behaves as if there is no if clause.

Usually, we implement the OMPIRBuilder and lowering to LLVMIR first in case some addition process needs to be done in lowering to MLIR. I would recommend you split this patch into two: 1. MLIR operation def (can be merged now) 2. lowering to MLIR (delayed after supporting OMPIRBuilder and lowering to LLVMIR). It's better to verify that the generated MLIR for if clause of SIMD construct in this patch works OK in OMPIRBuilder.

Is it OK to add a return failure in the translation of SIMD op if there is an if clause operand? Links below to the translation place of SIMD op and an existing return failure.
https://github.com/llvm/llvm-project/blob/e6ff553979e850eeb7f0bbe77deab1c88fc764b3/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp#L903
https://github.com/llvm/llvm-project/blob/e6ff553979e850eeb7f0bbe77deab1c88fc764b3/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp#L570

Also, I noticed there should be TODO for other clauses of SIMD construct. I think the lowering of clauses really should be refactored. @shraiysh Do you plan to continue that work? If yes, please add TODO for clauses of SIMD construct.

For now, may be @domada can add a static function that returns an MLIR value given an If clause. This static function can be later moved to the refactored class.

peixin added a comment.Jul 5 2022, 5:16 AM

Is it OK to add a return failure in the translation of SIMD op if there is an if clause operand? Links below to the translation place of SIMD op and an existing return failure.
https://github.com/llvm/llvm-project/blob/e6ff553979e850eeb7f0bbe77deab1c88fc764b3/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp#L903
https://github.com/llvm/llvm-project/blob/e6ff553979e850eeb7f0bbe77deab1c88fc764b3/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp#L570

Good point. This is also one option.

Also, I noticed there should be TODO for other clauses of SIMD construct. I think the lowering of clauses really should be refactored. @shraiysh Do you plan to continue that work? If yes, please add TODO for clauses of SIMD construct.

For now, may be @domada can add a static function that returns an MLIR value given an If clause. This static function can be later moved to the refactored class.

Agree.

flang/lib/Lower/OpenMP.cpp
839

Sorry, I mean the following:

for (const auto &clause : loopOpClauseList.v) {
  if (const auto &scheduleClause =
          std::get_if<Fortran::parser::OmpClause::Schedule>(&clause.u)) {
    if (const auto &chunkExpr =
            std::get<std::optional<Fortran::parser::ScalarIntExpr>>(
                scheduleClause->v.t)) {
      if (const auto *expr = Fortran::semantics::GetExpr(*chunkExpr)) {
        scheduleChunkClauseOperand =
            fir::getBase(converter.genExprValue(*expr, stmtCtx));
      }
    }
  } else if (const auto &ifClause = std::get_if<Fortran::parser::OmpClause::If>(&clause.u)) {
    auto &expr = std::get<Fortran::parser::ScalarLogicalExpr>(ifClause->v.t);
    mlir::Value ifVal = fir::getBase(
        converter.genExprValue(*Fortran::semantics::GetExpr(expr), stmtCtx));
    ifClauseOperand = firOpBuilder.createConvert(
        currentLocation, firOpBuilder.getI1Type(), ifVal);
  }
}
domada updated this revision to Diff 442510.Jul 6 2022, 4:30 AM

Added check for unsupported lowering if clause to LLVM IR.
Refactor Flang-MLIR lowering. Extracted function for handling if clause.

peixin accepted this revision.Jul 6 2022, 4:53 AM

LGTM

This revision is now accepted and ready to land.Jul 6 2022, 4:53 AM
kiranchandramohan accepted this revision.Jul 6 2022, 5:31 AM

LGTM. Thanks and Welcome to the OpenMP for Flang team!

Nit: You can also probably mention in the commit message that,
-> the patch also lowers the clause from the Flang parse-tree to MLIR, the translation to LLVM IR will be added in a later patch.
-> Reuses the custom handling for LoopControl in the assembly-format.
-> Includes the inclusive attribute.

This revision was automatically updated to reflect the committed changes.