[OpenMP] Support operation conversion to LLVM for threadprivate directive

[peixin]

Summary

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

[peixin]

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]

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

[awarzynski]

@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

[peixin]

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.

[peixin]

In D124610#3479961, @kiranchandramohan wrote:

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?

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

[kiranchandramohan]

In D124610#3481979, @peixin wrote:

In D124610#3479961, @kiranchandramohan wrote:

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

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

[peixin]

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]

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?

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

[awarzynski]

In D124610#3484526, @peixin wrote:

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.

[peixin]

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

@awarzynski Thanks for creating the issue.

[peixin]

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.

[kiranchandramohan]

In D124610#3505572, @peixin wrote:

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?

[peixin]

In D124610#3531025, @kiranchandramohan wrote:

In D124610#3505572, @peixin wrote:

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.

[kiranchandramohan]

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

817

Var/Variable SGTM.

[shraiysh]

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.

866

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.

871

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

[kiranchandramohan]

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.

[shraiysh]

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.

[peixin]

In D124610#3531283, @shraiysh wrote:

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]

rebase and address comments.

[kiranchandramohan]

LGTM.

[peixin]

@shraiysh Is this OK to you now?

[shraiysh]

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

[Mogball]

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

66

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

[NathanielMcVicar]

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

[shraiysh]

Fixed in D126556

[schweitz]

In D124610#3540106, @shraiysh wrote:

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

[shraiysh]

In D124610#3542998, @schweitz wrote:

In D124610#3540106, @shraiysh wrote:

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?

[schweitz]

In D124610#3543030, @shraiysh wrote:

In D124610#3542998, @schweitz wrote:

In D124610#3540106, @shraiysh wrote:

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]

In D124610#3560448, @schweitz wrote:

In D124610#3543030, @shraiysh wrote:

In D124610#3542998, @schweitz wrote:

In D124610#3540106, @shraiysh wrote:

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.

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

let arguments = (ins OpenMP_PointerLikeType:$x,
                        OptionalAttr<MemoryOrderKindAttr>:$memory_order_val);
   let assemblyFormat = [{
     $v `=` $x 
     oilist( `memory_order` `(` custom<ClauseAttr>($memory_order_val) `)`
           | `hint` `(` custom<SynchronizationHint>($hint_val) `)`)
     `:` type($x) attr-dict
   }];
   let hasVerifier = 1;
+  let extraClassDeclaration = [{
+    /// The number of variable operands.
+    unsigned getNumVariableOperands() {
+      assert(x() && "expected 'x' operand");
+      assert(v() && "expected 'v' operand");
+      return 2;
+    }
+
+    /// The i-th variable operand passed.
+    Value getVariableOperand(unsigned i) {
+      assert(0 <= i < 2 && "invalid index position for an operand");
+      return i == 0 ? x() : v();
+    }
+  }];
 }
 
 def AtomicWriteOp : OpenMP_Op<"atomic.write"> {
 
   let summary = "performs an atomic write";
 
   let description = [{
     This operation performs an atomic write.
def AtomicWriteOp : OpenMP_Op<"atomic.write"> {
   let assemblyFormat = [{
     $address `=` $value
     oilist( `hint` `(` custom<SynchronizationHint>($hint_val) `)`
           | `memory_order` `(` custom<ClauseAttr>($memory_order_val) `)`)
     `:` type($address) `,` type($value)
     attr-dict
   }];
   let hasVerifier = 1;
+  let extraClassDeclaration = [{
+    /// The number of variable operands.
+    unsigned getNumVariableOperands() {
+      assert(address() && "expected address operand");
+      assert(value() && "expected value operand");
+      return 2;
+    }
+
+    /// The i-th variable operand passed.
+    Value getVariableOperand(unsigned i) {
+      assert(0 <= i < 2 && "invalid index position for an operand");
+      return i == 0 ? address() : value();
+    }
+  }];
 }
 
 def AtomicUpdateOp : OpenMP_Op<"atomic.update",
                                [SingleBlockImplicitTerminator<"YieldOp">]> {
 
   let summary = "performs an atomic update";
 
   let description = [{
let description = [{
     the original variable.
   }];
 
   let arguments = (ins OpenMP_PointerLikeType:$sym_addr);
   let results = (outs OpenMP_PointerLikeType:$tls_addr);
   let assemblyFormat = [{
     $sym_addr `:` type($sym_addr) `->` type($tls_addr) attr-dict
   }];
+  let extraClassDeclaration = [{
+    /// The number of variable operands.
+    unsigned getNumVariableOperands() {
+      assert(sym_addr() && "expected one variable operand");
+      return 1;
+    }
+
+    /// The i-th variable operand passed.
+    Value getVariableOperand(unsigned i) {
+      assert(i == 0 && "invalid index position for an operand");
+      return sym_addr();
+    }
+  }];
 }
 
 //===----------------------------------------------------------------------===//
 // 2.18.1 Cancel Construct
 //===----------------------------------------------------------------------===//
 def CancelOp : OpenMP_Op<"cancel"> {
   let summary = "cancel directive";
   let description = [{

mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp

 };
 
 template <typename T>
 struct RegionLessOpConversion : public ConvertOpToLLVMPattern<T> {
   using ConvertOpToLLVMPattern<T>::ConvertOpToLLVMPattern;
   LogicalResult
   matchAndRewrite(T curOp, typename T::Adaptor adaptor,
                   ConversionPatternRewriter &rewriter) const override {
-    rewriter.replaceOpWithNewOp<T>(curOp, TypeRange(), adaptor.getOperands(),
+    TypeConverter *converter = ConvertToLLVMPattern::getTypeConverter();
+    SmallVector<Type> resTypes;
+    if (failed(converter->convertTypes(curOp->getResultTypes(), resTypes)))
+      return failure();
+    SmallVector<Value> convertedOperands;
+    for (unsigned idx = 0; idx < curOp.getNumVariableOperands(); ++idx) {
+      Value originalVariableOperand = curOp.getVariableOperand(idx);
+      if (!originalVariableOperand)
+        return failure();
+      if (originalVariableOperand.getType().isa<MemRefType>()) {
+        // TODO: Support memref type in variable operands
+        rewriter.notifyMatchFailure(curOp, "memref is not supported yet");
+      } else {
+        convertedOperands.emplace_back(adaptor.getOperands()[idx]);
+      }
+    }
+    rewriter.replaceOpWithNewOp<T>(curOp, resTypes, convertedOperands,
                                    curOp->getAttrs());
     return success();
   }
 };
 } // namespace
 
 void mlir::configureOpenMPToLLVMConversionLegality(
     ConversionTarget &target, LLVMTypeConverter &typeConverter) {
   target.addDynamicallyLegalOp<mlir::omp::ParallelOp, mlir::omp::WsLoopOp,
                                mlir::omp::MasterOp>(
       [&](Operation *op) { return typeConverter.isLegal(&op->getRegion(0)); });
   target
-      .addDynamicallyLegalOp<mlir::omp::AtomicReadOp, mlir::omp::AtomicWriteOp>(
-          [&](Operation *op) {
-            return typeConverter.isLegal(op->getOperandTypes());
-          });
+      .addDynamicallyLegalOp<mlir::omp::AtomicReadOp, mlir::omp::AtomicWriteOp,
+                             mlir::omp::ThreadprivateOp>([&](Operation *op) {
 }
 
 void mlir::populateOpenMPToLLVMConversionPatterns(LLVMTypeConverter &converter,
                                                   RewritePatternSet &patterns) {
   patterns.add<RegionOpConversion<omp::MasterOp>,
                RegionOpConversion<omp::ParallelOp>,
                RegionOpConversion<omp::WsLoopOp>,
                RegionLessOpConversion<omp::AtomicReadOp>,
-               RegionLessOpConversion<omp::AtomicWriteOp>>(converter);
+               RegionLessOpConversion<omp::AtomicWriteOp>,
+               RegionLessOpConversion<omp::ThreadprivateOp>>(converter);
 }
 
 namespace {
 struct ConvertOpenMPToLLVMPass
     : public ConvertOpenMPToLLVMBase<ConvertOpenMPToLLVMPass> {
   void runOnOperation() override;
 };
 } // namespace

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

-// RUN: mlir-opt -convert-openmp-to-llvm %s  -split-input-file | FileCheck %s
+// RUN: mlir-opt -convert-openmp-to-llvm -split-input-file %s | FileCheck %s
 
 // CHECK-LABEL: llvm.func @master_block_arg
 func.func @master_block_arg() {
   // CHECK: omp.master
   omp.master {
   // CHECK-NEXT: ^[[BB0:.*]](%[[ARG1:.*]]: i64, %[[ARG2:.*]]: i64):
   ^bb0(%arg1: index, %arg2: index):
     // CHECK-DAG: %[[CAST_ARG1:.*]] = builtin.unrealized_conversion_cast %[[ARG1]] : i64 to index
     // CHECK-DAG: %[[CAST_ARG2:.*]] = builtin.unrealized_conversion_cast %[[ARG2]] : i64 to index
     // CHECK-NEXT: "test.payload"(%[[CAST_ARG1]], %[[CAST_ARG2]]) : (index, index) -> ()
     "test.payload"(%arg1, %arg2) : (index, index) -> ()
     omp.terminator
   }
   return
 }
 
+// -----
+
 // CHECK-LABEL: llvm.func @branch_loop
 func.func @branch_loop() {
   %start = arith.constant 0 : index
   %end = arith.constant 0 : index
   // CHECK: omp.parallel
   omp.parallel {
     // CHECK-NEXT: llvm.br ^[[BB1:.*]](%{{[0-9]+}}, %{{[0-9]+}} : i64, i64
     cf.br ^bb1(%start, %end : index, index)
^bb3:
     omp.barrier
     omp.taskwait
     omp.taskyield
     omp.terminator
   }
   return
 }
 
+// -----
+
 // CHECK-LABEL: @wsloop
 // CHECK: (%[[ARG0:.*]]: i64, %[[ARG1:.*]]: i64, %[[ARG2:.*]]: i64, %[[ARG3:.*]]: i64, %[[ARG4:.*]]: i64, %[[ARG5:.*]]: i64)
 func.func @wsloop(%arg0: index, %arg1: index, %arg2: index, %arg3: index, %arg4: index, %arg5: index) {
   // CHECK: omp.parallel
   omp.parallel {
     // CHECK: omp.wsloop for (%[[ARG6:.*]], %[[ARG7:.*]]) : i64 = (%[[ARG0]], %[[ARG1]]) to (%[[ARG2]], %[[ARG3]]) step (%[[ARG4]], %[[ARG5]]) {
     "omp.wsloop"(%arg0, %arg1, %arg2, %arg3, %arg4, %arg5) ({
     ^bb0(%arg6: index, %arg7: index):  
       // CHECK-DAG: %[[CAST_ARG6:.*]] = builtin.unrealized_conversion_cast %[[ARG6]] : i64 to index
       // CHECK-DAG: %[[CAST_ARG7:.*]] = builtin.unrealized_conversion_cast %[[ARG7]] : i64 to index
       // CHECK: "test.payload"(%[[CAST_ARG6]], %[[CAST_ARG7]]) : (index, index) -> ()
       "test.payload"(%arg6, %arg7) : (index, index) -> ()
       omp.yield
     }) {operand_segment_sizes = dense<[2, 2, 2, 0, 0, 0, 0]> : vector<7xi32>} : (index, index, index, index, index, index) -> ()
     omp.terminator
   }
   return
 }
+
+// -----
+
+// CHECK-LABEL: @atomic_write
+// CHECK: (%[[ARG0:.*]]: !llvm.ptr<i32>)
+// CHECK: %[[VAL0:.*]] = llvm.mlir.constant(1 : i32) : i32
+// CHECK: omp.atomic.write %[[ARG0]] = %[[VAL0]] hint(none) memory_order(relaxed) : !llvm.ptr<i32>, i32
+func.func @atomic_write(%a: !llvm.ptr<i32>) -> () {
+  %1 = arith.constant 1 : i32
+  omp.atomic.write %a = %1 hint(none) memory_order(relaxed) : !llvm.ptr<i32>, i32
+  return
+}
+
+// -----
+
+// CHECK-LABEL: @atomic_read
+// CHECK: (%[[ARG0:.*]]: !llvm.ptr<i32>, %[[ARG1:.*]]: !llvm.ptr<i32>)
+// CHECK: omp.atomic.read %[[ARG1]] = %[[ARG0]] memory_order(acquire) hint(contended) : !llvm.ptr<i32>
+func.func @atomic_read(%a: !llvm.ptr<i32>, %b: !llvm.ptr<i32>) -> () {
+  omp.atomic.read %b = %a memory_order(acquire) hint(contended) : !llvm.ptr<i32>
+  return
+}
+
+// -----
+
+// CHECK-LABEL: @threadprivate
+// CHECK: (%[[ARG0:.*]]: !llvm.ptr<i32>)
+// CHECK: %[[VAL0:.*]] = omp.threadprivate %[[ARG0]] : !llvm.ptr<i32> -> !llvm.ptr<i32>
+func.func @threadprivate(%a: !llvm.ptr<i32>) -> () {
+  %1 = omp.threadprivate %a : !llvm.ptr<i32> -> !llvm.ptr<i32>
+  return
+}