This supports the operation conversion for threadprivate directive. The
support for memref type conversion is not implemented.
Details
Diff Detail
Event Timeline
As @awarzynski suggestted, there are two more choices for integration tests: llvm-test-suite and cross-project-tests. Other thoughts and suggestions for OpenMP workflow test?
I think this change in MLIR is better checked in MLIR itself by writing a conversion for the memref type similar to what is done in OpenACC (https://github.com/llvm/llvm-project/blob/05b0a498329c4b5db367120e5c9358bb74346131/mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp#L100)
Note that the memref type is already added to the OpenMP Pointer model accepted by threadprivate and atomic operations.
https://github.com/llvm/llvm-project/blob/05b0a498329c4b5db367120e5c9358bb74346131/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp#L59
@peixin Many thanks for extracting this into a separate patch. The end goal is much clearer now :)
Based just on the tests, it looks like you want to verify the LLVM IR generated from the input Fortran/OpenMP code - this covers both lowering and code-gen in Flang (i.e. 2 stages/components). This makes me wonder:
- Is there anything here that cannot be tested in two separate "component" tests (Fortran -> MLIR, MLIR -> LLVM IR)?
- Given that there are component tests for this (perhaps my assumption is wrong?), would it be possible to simplify these tests to the bare minimum? Currently, there's a lot of noise in these tests and it is hard to identify the core thing that you want to verify.
Or, more broadly, how do we decide that a certain functionality requires a test that combines lowering and code-gen? If we do it for all of Flang (why not?), then we will end-up with many tests to maintain.
flang/test/Integration/OpenMPLLVM/threadprivate-use-association.f90 | ||
---|---|---|
1 ↗ | (On Diff #425759) | [nit] Lowering + code-gen |
Or, more broadly, how do we decide that a certain functionality requires a test that combines lowering and code-gen?
I am afraid we don't know how to decide. The only benefit of the test combining lowering and code-gen currently that I know is to avoid the hidden bugs.
If we do it for all of Flang (why not?), then we will end-up with many tests to maintain.
That's true. Since there are projects starting to test applications including OpenMP, it seems not to be worth to take the integration tests in llvm-project now.
- The function code in this patch can support all Fortran data types conversion. The curOp.getType() for integer pointer in Fortran is !fir.ref<!fir.box<!fir.ptr<i32>>>, which will be converted into !llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>> by the ConvertToLLVMPattern::getTypeConverter(). Why do we want to support the memref type here? Is that because the memref type is the common type in MLIR and can be used not only for Fortran?
- BTW, pointer and allocatables variables in atomic and threadprivate are not supported since the upstream of FIR type conversion has not been finished. Once it's done, pointer and allocatables variables in atomic and threadprivate will be supported automatically.
Yes, memref is the only in-tree MLIR dialect data type for which the Conversion/OpenMPToLLVM apply, in the case of threadprivate and atomic ops. memref needs some special handling because it converts to a struct and not a pointer and threadprivate/atomic ops do not accept that.
Thinking about it a bit more, we have two options:
- Add conversion support for memref and add this legality check in mlir openmp.
- Since (at the moment) it is only FIR+OpenMP that needs this legality check then it can be added to the conversion pass in Flang (https://github.com/llvm/llvm-project/blob/14869bd2dfabb7a808e57e17dd45eef7665dd737/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L3376). We might need to put that in a function in a separate file and call it from CodeGen.cpp
- BTW, pointer and allocatables variables in atomic and threadprivate are not supported since the upstream of FIR type conversion has not been finished. Once it's done, pointer and allocatables variables in atomic and threadprivate will be supported automatically.
Hmm, I thought type-conversion upstreaming is complete, could you let me know what type is pending?
Ideally, as @awarzynski seem to be suggesting this should be only tested by what is needed. i.e input should be FIR+OpenMP and output check should be for LLVM+OpenMP. No need to check for LLVM IR here.
I think we can move the discussion about integration tests (Fortran -> LLVM dialect, Fortran -> LLVM IR) and end-to-end tests to two different discourse threads possibly after a discussion in the OpenMP Flang meeting.
Add conversion support for memref and add this legality check in mlir openmp.
I prefer this option since OpenACC already uses this. It's better to keep consistency.
Hmm, I thought type-conversion upstreaming is complete, could you let me know what type is pending?
pointer and allocatable global ops. See the following:
program m integer, save, pointer :: x !$omp atomic write hint(0) x = 8 end
$ flang-new -fc1 -emit-llvm -fopenmp atomic.f90 error: loc("./atomic.f90":2:29): 'llvm.mlir.global' op initializer region type '!llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>>' does not match global type '!llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>' error: Lowering to LLVM IR failed
integer, pointer :: m !$omp threadprivate(m) print *, m end
$ flang-new -fc1 -emit-llvm -fopenmp threadprivate-pointer-allocatable.f90 error: 'llvm.mlir.global' op initializer region type '!llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>>' does not match global type '!llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>' error: Lowering to LLVM IR failed
$ flang-new -fc1 -emit-fir -fopenmp atomic.f90 $ tco atomic.mlir loc("atomic.mlir":8:3): error: 'llvm.mlir.global' op initializer region type '!llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>>' does not match global type '!llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>' llvm.func @_QQmain() { %0 = llvm.mlir.constant(8 : i32) : i32 %1 = llvm.mlir.addressof @_QFEx : !llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>> omp.atomic.write %1 = %0 hint(none) : !llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>>, i32 llvm.return } "llvm.mlir.global"() ({ %0 = "llvm.mlir.null"() : () -> !llvm.ptr<i32> %1 = "llvm.mlir.undef"() : () -> !llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)> ... %18 = "llvm.insertvalue"(%15, %17) {position = [6 : i32]} : (!llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>, i8) -> !llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)> %19 = "llvm.bitcast"(%0) : (!llvm.ptr<i32>) -> !llvm.ptr<i32> %20 = "llvm.insertvalue"(%18, %19) {position = [0 : i32]} : (!llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>, !llvm.ptr<i32>) -> !llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)> %21 = "builtin.unrealized_conversion_cast"(%20) : (!llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>) -> !llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>> "llvm.return"(%21) : (!llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>>) -> () }) {global_type = !llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>, linkage = #llvm.linkage<internal>, sym_name = "_QFEx"} : () -> () FAILED: atomic.mlir
Ideally, as @awarzynski seem to be suggesting this should be only tested by what is needed. i.e input should be FIR+OpenMP and output check should be for LLVM+OpenMP. No need to check for LLVM IR here.
I think we can move the discussion about integration tests (Fortran -> LLVM dialect, Fortran -> LLVM IR) and end-to-end tests to two different discourse threads possibly after a discussion in the OpenMP Flang meeting.
Agree.
- Without OpenMP, the following test case still fails.
integer, save, pointer :: x x = 1 end
$ flang-new test.f90 error: loc("./atomic-read.f90":2:29): 'llvm.mlir.global' op initializer region type '!llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>>' does not match global type '!llvm.struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>' error: Lowering to LLVM IR failed error: loc("./atomic-read.f90":2:29): cannot be converted to LLVM IR: missing `LLVMTranslationDialectInterface` registration for dialect for op: builtin.unrealized_conversion_cast error: loc("./atomic-read.f90":2:29): unemittable constant value error: failed to create the LLVM module
@schweitz @jeanPerier Any idea what's going on here?
- When I tried to convert memref type for OpenMP, I found it seems that MemRefDescriptor can be used (check https://github.com/llvm/llvm-project/blob/43c146c96d8e4607266f2c2ef74c17d4170fc248/mlir/include/mlir/Conversion/LLVMCommon/MemRefBuilder.h#L30-L112). Why do you create one similar DataDescriptor for the memref type conversion instead of using MemRefDescriptor? @clementval
I've had a quick look: https://github.com/llvm/llvm-project/issues/55210.
@awarzynski Thanks for creating the issue.
I don't figure out why OpenACC uses the DataDescriptor for memref conversion. I tested the following test case in both fir-dev and upstream LLVM as follows:
$ cat enterdata.f90 subroutine acc_enter_data integer :: async = 1 real, dimension(10, 10) :: a, b, c real, pointer :: d !$acc enter data copyin(a) create(b) attach(d) end $ bbc -emit-fir -fopenacc enterdata.f90 $ tco enterdata.mlir loc("enterdata.mlir":9:3): error: failed to legalize operation 'acc.enter_data' func.func @_QPacc_enter_data() { %0 = fir.alloca !fir.array<10x10xf32> {bindc_name = "a", uniq_name = "_QFacc_enter_dataEa"} %1 = fir.alloca !fir.array<10x10xf32> {bindc_name = "b", uniq_name = "_QFacc_enter_dataEb"} %2 = fir.alloca !fir.array<10x10xf32> {bindc_name = "c", uniq_name = "_QFacc_enter_dataEc"} %3 = fir.alloca !fir.box<!fir.ptr<f32>> {bindc_name = "d", uniq_name = "_QFacc_enter_dataEd"} %4 = fir.alloca !fir.ptr<f32> {uniq_name = "_QFacc_enter_dataEd.addr"} %5 = fir.zero_bits !fir.ptr<f32> fir.store %5 to %4 : !fir.ref<!fir.ptr<f32>> acc.enter_data copyin(%0 : !fir.ref<!fir.array<10x10xf32>>) create(%1 : !fir.ref<!fir.array<10x10xf32>>) attach(%3 : !fir.ref<!fir.box<!fir.ptr<f32>>>) return } fir.global internal @_QFacc_enter_dataEasync : i32 { %c1_i32 = arith.constant 1 : i32 fir.has_value %c1_i32 : i32 } FAILED: enterdata.mlir
I also test the memref conversion in upstream MAIN as follows:
$ cat acc-conversion.mlir func.func @testenterdataop(%a: memref<10xf32>, %b: memref<10xf32>) -> () { acc.enter_data copyin(%a : memref<10xf32>) create(%b : memref<10xf32>) return } $ mlir-opt -convert-openacc-to-llvm -split-input-file acc-conversion.mlir | tco loc("<stdin>":5:10): error: failed to legalize operation 'builtin.unrealized_conversion_cast'
All in all, I don't know what data and data type should be converted into for memref, so I leave it as TODO for now. The non-memref type conversion can support all the test cases of Fortran code in D124226.
Note that the DataDescriptor is the structure to which all of the types map for the Data directive/clause. This includes the FIR types and the memref types. The translation understands the Datadescriptor. If you want more context then please go through https://reviews.llvm.org/D101504, https://reviews.llvm.org/D102170.
Note that Valentin has not been able to work on OpenACC for almost a year now due to upstreaming. He has probably not been able to complete the entire flow, the FIR patch is https://github.com/flang-compiler/f18-llvm-project/pull/915. Maybe once upstreaming is complete, he will get back to it. Also, note that Valentin is away currently and back only next month.
All in all, I don't know what data and data type should be converted into for memref, so I leave it as TODO for now. The non-memref type conversion can support all the test cases of Fortran code in D124226.
This is fine for now.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
817 | I think OpenACC was using the name data since it was specifically for the data construct or clause. Should we use a different name here? |
Thanks for these information.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
817 | Right, we should use a different name. How about using "Var"? As OpenMP Spec, for x and v in atomic statements, they are both scalar variables. For the list in threadprivate directive, it is named variables and named common block for fortran, and file-scope, namespace-scope, or static block-scope variables for C/C++. The common block can be taken as one struct variable. All in all, they are variables in general meaning. |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
817 | Var/Variable SGTM. |
Minor comments. Apologies for the delay.
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
817 | This method must always return 2. Maybe put an assert inside to check for existence of x() and v(), but this method returning anything other than 2 is an error. | |
821 | assert to check 0 < i < 2. | |
859 | same here. This must always return 2. Maybe assert internally to ensure the existence of address() and value() but anything other than 2 is an error. | |
864 | assert to check 0 < i < 2. | |
mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir | ||
78 | Probably an unrelated question but how is this file different from openmp-llvm.mlir (llvm-project/mlir/test/Target/LLVMIR/openmp-llvm.mlir)? |
mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir | ||
---|---|---|
78 | This file tests the conversion pass in https://github.com/llvm/llvm-project/blob/main/mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp. This is conversion to the llvm dialect i.e how to convert, code in OpenMP regions, operands, arguments etc and how to check that the conversion is legal. llvm-project/mlir/test/Target/LLVMIR/openmp-llvm.mlir contains tests for the translation code in https://github.com/llvm/llvm-project/blob/main/mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp. This is translation to llvm IR. |
Yes, this looks good to me for the time being as memref isn't supported. Support for memref can be added later.
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp | ||
---|---|---|
66 | FYI, the return value is nodiscard. This is causing build failures at HEAD. |
mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td | ||
---|---|---|
826 | I believe this and the below comparison are producing warnings (as errors) on the Windows bot in the generated code. C:\buildbot\mlir-x64-windows-ninja\build\tools\mlir\include\mlir/Dialect/OpenMP/OpenMPOps.h.inc(384): error C2220: the following warning is treated as an error C:\buildbot\mlir-x64-windows-ninja\build\tools\mlir\include\mlir/Dialect/OpenMP/OpenMPOps.h.inc(384): warning C4804: '<': unsafe use of type 'bool' in operation C:\buildbot\mlir-x64-windows-ninja\build\tools\mlir\include\mlir/Dialect/OpenMP/OpenMPOps.h.inc(604): warning C4804: '<': unsafe use of type 'bool' in operation |
You'll want to be careful. The MLIR memref type, when lowered to LLVM, is incompatible with Fortran descriptors and as such might cause some grief. (In FIR, it is a requirement to manage memory references in a way that supports varied Fortran calling conventions.)
IIUC, the concern arises when FIR Dialect types are used in the same IR file as MemRef types. There should be no problem having a memref lowering independent of FIR, right? i.e. Both (OpenMP + Memref -> LLVM) and (OpenMP + FIR -> LLVM) should not be tricky but (OpenMP + FIR + Memref -> LLVM) could be tricky. Is that correct?
Yes. As long as they can be kept mutually exclusive and there is no interaction, it should be ok. The tricky part may be in guaranteeing that mutual exclusion.
It seems that the type check is needed for OpenMP data-related constructs and clauses similar to legal conversion check in flang side if we want to make it safe.
I think OpenACC was using the name data since it was specifically for the data construct or clause. Should we use a different name here?