This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Support operation conversion to LLVM for threadprivate directive
ClosedPublic

Authored by peixin on Apr 28 2022, 6:20 AM.

Details

Summary

This supports the operation conversion for threadprivate directive. The
support for memref type conversion is not implemented.

Diff Detail

Event Timeline

peixin created this revision.Apr 28 2022, 6:20 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 28 2022, 6:20 AM
peixin requested review of this revision.Apr 28 2022, 6:20 AM

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?

kiranchandramohan requested changes to this revision.Apr 28 2022, 6:31 AM

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

This revision now requires changes to proceed.Apr 28 2022, 6:31 AM

@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:

  1. Is there anything here that cannot be tested in two separate "component" tests (Fortran -> MLIR, MLIR -> LLVM IR)?
  2. 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.

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

  1. 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?
  1. 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.

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

  1. 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?

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:

  1. Add conversion support for memref and add this legality check in mlir openmp.
  2. 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
  1. 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.

peixin added a comment.May 1 2022, 2:48 AM
  1. 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?

  1. 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
  1. 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?

I've had a quick look: https://github.com/llvm/llvm-project/issues/55210.

I've had a quick look: https://github.com/llvm/llvm-project/issues/55210.

@awarzynski Thanks for creating the issue.

peixin updated this revision to Diff 428598.May 11 2022, 2:45 AM
peixin retitled this revision from [WIP][OpenMP] Support operation conversion to LLVM for threadprivate directive to [OpenMP] Support operation conversion to LLVM for threadprivate directive.
peixin edited the summary of this revision. (Show Details)

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.

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:

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?

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:

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.

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.

Just to be sure, are the changes in flang/test/Lower/OpenMP/atomic*.f90 duplicated from D126169? If so, can we please remove that change from this before landing?

mlir/test/Conversion/OpenMPToLLVM/convert-to-llvmir.mlir
78

Right, thank you. Understood.

Just to be sure, are the changes in flang/test/Lower/OpenMP/atomic*.f90 duplicated from D126169? If so, can we please remove that change from this before landing?

Yes. Thanks for the notice. Will rebase and address the comments later.

peixin updated this revision to Diff 431638.May 24 2022, 4:07 AM

rebase and address comments.

peixin edited the summary of this revision. (Show Details)May 24 2022, 4:13 AM
This revision is now accepted and ready to land.May 26 2022, 4:06 AM

@shraiysh Is this OK to you now?

shraiysh accepted this revision.May 26 2022, 7:37 AM

Yes, this looks good to me for the time being as memref isn't supported. Support for memref can be added later.

Mogball added inline comments.
mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp
66

FYI, the return value is nodiscard. This is causing build failures at HEAD.

NathanielMcVicar added inline comments.
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

https://lab.llvm.org/buildbot/#/builders/13/builds/21441

Yes, this looks good to me for the time being as memref isn't supported. Support for memref can be added later.

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.)

Yes, this looks good to me for the time being as memref isn't supported. Support for memref can be added later.

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, this looks good to me for the time being as memref isn't supported. Support for memref can be added later.

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.

peixin added a comment.Jun 6 2022, 8:58 AM

Yes, this looks good to me for the time being as memref isn't supported. Support for memref can be added later.

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.

flang/test/Lower/OpenMP/atomic01.f90