This is an archive of the discontinued LLVM Phabricator instance.

[MLIR][OpenMP] Add support for ordered construct
ClosedPublic

Authored by peixin on Sep 17 2021, 6:32 PM.

Details

Summary

This patch supports the ordered construct in OpenMP dialect following
Section 2.19.9 of the OpenMP 5.1 standard. Also lowering to LLVM IR
using OpenMP IRBduiler. Lowering to LLVM IR for ordered simd directive
is not supported yet since LLVM optimization passes do not support it
for now.

Diff Detail

Event Timeline

peixin created this revision.Sep 17 2021, 6:32 PM
peixin requested review of this revision.Sep 17 2021, 6:32 PM
peixin updated this revision to Diff 373441.Sep 18 2021, 9:27 PM

Fix clang-tidy warning.

ftynse requested changes to this revision.Sep 23 2021, 1:30 AM
ftynse added inline comments.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
423

typo: workshwring

429

Nit: couldn't parse "it behaves that the THREADS"

429

typo: cluase

437
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
482

Nit: you don't need a ternary operator here, just do bool isDependSource = dependType == omp::ClauseDepend::dependsource.

491

Nit: please .reserve space in a vector before calling push_back in the loop.

504

Should this return failure() in presence of said directive?

506–508

It would be better to actually check bodyGenStatus after bodyGenCB is transitively called below.

This revision now requires changes to proceed.Sep 23 2021, 1:30 AM
peixin updated this revision to Diff 376134.Sep 30 2021, 2:11 AM

@ftynse Thanks for your comments and addressed them.

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

Fixed.

429

Fixed.

429

The previous description is not correct. Replaced the statement of behaves that the THREADS clause is specfied with the statement of behaves as if the THREADS clause is specified or no clause is specified. There is no need to add the threads attribute in MLIR. The omp ordered and omp ordered threads directive in fortran code will be lowered to the following mlir:

omp.ordered {
  omp.terminator
} // nothing here
437

Fixed.

mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp
482

Fixed.

491

Fixed.

504

Yes, this is better. Fixed.

506–508

Good point. Fixed by return bodyGenStatus instead of success();

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

Do we need both depend and depend_type_val? The presence of depend_type_val implies depend right?

451

For discussion:
The standalone ordered construct does not have a region. Should we split the ordered construct into two MLIR operations, something like the following?
omp.ordered_region
omp.ordered

@kiranchandramohan Thanks for the comments.

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

Considering that the MLIR operation will be split, the depend attribute will be deleted.

451

Yes, I agree. This makes more sense although IR generated will be the same. Let me do it.

peixin updated this revision to Diff 378118.Oct 8 2021, 12:58 AM

Split the ordered construct into two MLIR operations, OrderedOp and OrderedRegionOp.

peixin updated this revision to Diff 378121.Oct 8 2021, 1:03 AM

Delete depend UnitAttr in test cases.

LGTM. Apologies for the delay. Please wait for @ftynse to approve.

Once you submit, you can cherry-pick these patches to fir-dev and then rebase the lowering of the ordered construct there.

@kiranchandramohan OK. Thanks for your review.

shraiysh added a subscriber: shraiysh.EditedOct 16 2021, 4:50 AM

Not sure if this is feasible in MLIR, but can we verify that the ordered construct is within a worksharing-loop or simd construct, as specified in the standard?

Not sure if this is feasible in MLIR, but can we verify that the ordered construct is within a worksharing-loop or simd construct, as specified in the standard?

Thanks @shraiysh for raising this.
You can get a parentofType as done in the verifyReductionOp.
https://github.com/llvm/llvm-project/blob/48ce523a26b7a5a3dc4cff616c93ed951244746b/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp#L833

peixin updated this revision to Diff 380353.Oct 18 2021, 5:35 AM

Add the verifier of MLIR and the corresponding test cases.

Thanks @shraiysh and @kiranchandramohan for noticing the missed verifier.

SIMD Op is not implemented yet. I left TODO for ordered simd.

ftynse accepted this revision.Oct 18 2021, 6:58 AM
ftynse added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1105

Nit: you can put the variable definition inside the if (auto container = op->getPraentOfType)

1106

Nit: I would put braces around the outer loop to avoid the potential dangling-else footgun.

This revision is now accepted and ready to land.Oct 18 2021, 6:58 AM

Some attributes are expected in the attribute dictionary. This is different from the way it is done for other operations (nowait, default for ParallelOp and collapse, order, ordered clauses for WsLoopOp). I do not have a preference for either way, but it should probably be uniform across the operations. Maybe others can advise on which format to opt for.

Some attributes are expected in the attribute dictionary. This is different from the way it is done for other operations (nowait, default for ParallelOp and collapse, order, ordered clauses for WsLoopOp). I do not have a preference for either way, but it should probably be uniform across the operations. Maybe others can advise on which format to opt for.

+1 for uniformity between the ops in the same dialects.

peixin updated this revision to Diff 380578.Oct 18 2021, 9:22 PM

Thanks @ftynse for the code format advice. Fixed.

Thanks @shraiysh @clementval for the operation uniformity advice. Add the depend_type and simd in assemblyFormat so that the operation keeps the similar format as others. I would rather leave num_loops_val as appendant. It is different as collapse and ordered in wsloop construct, which are clauses from fortran code. The num_loops_val does not exist in fortran code and it is helper to transform MLIR to LLVMIR. Take the following as one example:

!$omp depend(sink: i)

I think the MLIR format of omp.ordered depend_type("dependsink") depend_vec(%arg0: i64) {num_loops_val = 1 : i64} looks more reasonable than omp.ordered depend_type("dependsink") depend_vec(%arg0: i64) num_loops_val (1 : i64). Is this OK with you @shraiysh @clementval ?

Thanks @shraiysh @clementval for the operation uniformity advice. Add the depend_type and simd in assemblyFormat so that the operation keeps the similar format as others. I would rather leave num_loops_val as appendant. It is different as collapse and ordered in wsloop construct, which are clauses from fortran code. The num_loops_val does not exist in fortran code and it is helper to transform MLIR to LLVMIR. Take the following as one example:

!$omp depend(sink: i)

I think the MLIR format of omp.ordered depend_type("dependsink") depend_vec(%arg0: i64) {num_loops_val = 1 : i64} looks more reasonable than omp.ordered depend_type("dependsink") depend_vec(%arg0: i64) num_loops_val (1 : i64). Is this OK with you @shraiysh @clementval ?

I think this format omp.ordered depend_type("dependsink") depend_vec(%arg0: i64) {num_loops_val = 1 : i64} is fine.

Thanks @shraiysh @clementval for the operation uniformity advice. Add the depend_type and simd in assemblyFormat so that the operation keeps the similar format as others. I would rather leave num_loops_val as appendant. It is different as collapse and ordered in wsloop construct, which are clauses from fortran code. The num_loops_val does not exist in fortran code and it is helper to transform MLIR to LLVMIR. Take the following as one example:

!$omp depend(sink: i)

I think the MLIR format of omp.ordered depend_type("dependsink") depend_vec(%arg0: i64) {num_loops_val = 1 : i64} looks more reasonable than omp.ordered depend_type("dependsink") depend_vec(%arg0: i64) num_loops_val (1 : i64). Is this OK with you @shraiysh @clementval ?

I think this format omp.ordered depend_type("dependsink") depend_vec(%arg0: i64) {num_loops_val = 1 : i64} is fine.

+1. I think this is fine.

Is it OK to land this patch now?

clementval accepted this revision.Oct 19 2021, 1:19 AM

Thank you for updating the patch @peixin. I have a few comments.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
442–446

The clauses are not order-independent at the moment (there was a discussion on the slack channel about this and they decided to support order-independence for clauses in OpenMP dialect). Another thing is that the values of "dependsink" and "dependsource" are expected to come with quotations marks. This is different from the way it is done for other clause values (proc_bind and default clauses expect their values like a keyword). I think you might have to add custom parser/printers for this operation for now.

There is an RFC to support order independence within assemblyFormat. Once that goes through, this might get simplified.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1105

AFAIK ordered clause with region is also not allowed outside of a worksharing loop/simd construct. That is, like verifyOrderedOp a similar check for if(!container || ...) should be here. I am not sure about this restriction though, so please feel free to correct me if I am mistaken.

peixin added inline comments.Oct 19 2021, 1:59 AM
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
442–446

Keep the depend_type_val, depend_vec_vars, and num_loops_val in order is better since it is more like the fortran source code. This case is different from others since these three arguments are generated from one clause, and they are not indepentdent.

The depend_type_val is a little bit different from the default and proc_bind clauses, which are keyword of the clauses. The keyword of depend_type_val is not the string "dependsource" or "dependsink". Instead, it is depend_type, which has the similar format as default and proc_bind clauses. This depend clause is special since it has two keywords followed by one vector and multiple clauses can exist. These three arguments actully describe one clause. I think the current format is good enough to match the format of fortran code. What do you think?

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1105

You understand it correctly. The problem is that the current compiler cannot do interprocedural analysis. So for the following case, if (!container) should not be there.

subroutine sub1()
  !$omp ordered threads
  !$omp end ordered
end
subroutine sub2()
  !$omp do ordered
  ...
  call sub1()
  ...
  !$omp end do
end

LGTM.

mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td
442–446

Okay, thanks for the clarification about the ordering.

About the proc_bind clause, I meant that the syntax there is omp.parallel proc_bind(close) and not omp.parallel proc_bind("close"). But I think this is okay because we can use assemblyFormat if we allow the quotation marks. Writing custom parsers/printers for this alone is not worth it IMO.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1105

Oh okay, sounds good. Should this check then be removed from the OrderedOp too?

peixin added inline comments.Oct 19 2021, 2:29 AM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1105

No. The special case of OrderedOp ordered depend(source) doesn't know the number of loops, so there must be one wsloop in the same procedure. That is, the above code for ordered depend(source) cannot be lowered since the loop number info is missed. So we should give user one semantic error, which is also done by gfortran.

shraiysh accepted this revision.Oct 19 2021, 3:05 AM
shraiysh added inline comments.
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
1105

Okay, that makes sense. Thanks for the explanation.

Is it OK to land this patch now?

I will land this patch now, and cherry-pick this and the other two ordered construct related patches to fir-dev.

This revision was automatically updated to reflect the committed changes.
peixin updated this revision to Diff 381161.Oct 21 2021, 12:44 AM

Rebase this patch and delete the inclusive attribute to avoid CI fail.

peixin reopened this revision.Oct 21 2021, 12:44 AM
This revision is now accepted and ready to land.Oct 21 2021, 12:44 AM
This revision was automatically updated to reflect the committed changes.