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.
Details
Diff Detail
Event Timeline
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. |
@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: |
@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. |
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.
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
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.
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.
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.
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. |
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? |
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. |
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp | ||
---|---|---|
1105 | Okay, that makes sense. Thanks for the explanation. |
I will land this patch now, and cherry-pick this and the other two ordered construct related patches to fir-dev.
typo: workshwring