[mlir][openmp] Add check for types of operands in omp.atomic.write

[shraiysh]

Summary

This patch makes sure that the address dereferences to value in
omp.atomic.write operation.

[peixin]

It seems that this check is unnecessary. If you check the following example,

func.func @omp_atomic_write(%addr : !llvm.ptr<!llvm.ptr<i32>>, %val : i32) {
  omp.atomic.write %addr = %val : !llvm.ptr<!llvm.ptr<i32>>, i32
  return
}

$ tco atomic.mlir 
tco: /home/qpx/compilers/llvm-community/debug-mode/llvm-project/llvm/lib/IR/Instructions.cpp:1510: void llvm::StoreInst::AssertOK(): Assertion `cast<PointerType>(getOperand(1)->getType()) ->isOpaqueOrPointeeTypeMatches(getOperand(0)->getType()) && "Ptr must be a pointer to Val type!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

This test case exists since there is one bug when lowering to MLIR. Instead of getting an error, the current output letting users submit one bug is more reasonable. What do you think?

[shraiysh]

mlir-opt accepts this input without this patch. Irrespective of the error in translation, this is invalid MLIR and so mlir-opt must report an error.

[kiranchandramohan]

Agree with @shraiysh. This is invalid MLIR and we should catch it at this layer.

[peixin]

I have two concerns for this:

subroutine atomic_write_pointer()
  integer, pointer :: x
  !$omp atomic write
    x = 1
end

For the above test case, without this patch, it reports the following:

$ flang-new -fopenmp write-atomic.f90 
flang-new: /home/.../llvm-project/llvm/lib/IR/Instructions.cpp:1510: void llvm::StoreInst::AssertOK(): Assertion `cast<PointerType>(getOperand(1)->getType()) ->isOpaqueOrPointeeTypeMatches(getOperand(0)->getType()) && "Ptr must be a pointer to Val type!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

Users will know this is the bug in flang, not their code problem. With this patch, it reports the following:

$ flang-new -fopenmp write-atomic.f90 
error: loc("./write-atomic.f90":1:1): address must dereference to value type
error: verification of lowering to FIR failed

Users will be confused if this is the problem of the compiler or their code. Does "address must dereference to value type" mean I should only use value instead of pointer? Can I only use integer :: x here?

2. I don't know the purpose of mlir-opt. Does it test the operation conversion here? If yes, I think semantic analysis should not be covered here. The semantic analysis is the job in frontend. This is codegen.

This reminds me that some errors may should not be reported in mlir. The errors reporting will make users be confused if there is some bug in frontend. Maybe an assertion is better than an error.

[kiranchandramohan]

In D126272#3536568, @peixin wrote:

I have two concerns for this:

subroutine atomic_write_pointer()
  integer, pointer :: x
  !$omp atomic write
    x = 1
end

For the above test case, without this patch, it reports the following:

$ flang-new -fopenmp write-atomic.f90 
flang-new: /home/.../llvm-project/llvm/lib/IR/Instructions.cpp:1510: void llvm::StoreInst::AssertOK(): Assertion `cast<PointerType>(getOperand(1)->getType()) ->isOpaqueOrPointeeTypeMatches(getOperand(0)->getType()) && "Ptr must be a pointer to Val type!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

Users will know this is the bug in flang, not their code problem. With this patch, it reports the following:

$ flang-new -fopenmp write-atomic.f90 
error: loc("./write-atomic.f90":1:1): address must dereference to value type
error: verification of lowering to FIR failed

Users will be confused if this is the problem of the compiler or their code. Does "address must dereference to value type" mean I should only use value instead of pointer? Can I only use integer :: x here?

2. I don't know the purpose of mlir-opt. Does it test the operation conversion here? If yes, I think semantic analysis should not be covered here. The semantic analysis is the job in frontend. This is codegen.

This reminds me that some errors may should not be reported in mlir. The errors reporting will make users be confused if there is some bug in frontend. Maybe an assertion is better than an error.

In general, the OpenMP dialect should not be tied to flang. Other frontends can use it (infact polygeist uses it https://github.com/wsmoses/Polygeist/blob/2182fa1b88c621e49b3289628f7177f2f9b48718/test/polygeist-opt/openmpopt.mlir) and also users can hand-code in the OpenMP dialect. Hence, we should produce appropriate error messages for incorrect usage.

[peixin]

In general, the OpenMP dialect should not be tied to flang. Other frontends can use it (infact polygeist uses it https://github.com/wsmoses/Polygeist/blob/2182fa1b88c621e49b3289628f7177f2f9b48718/test/polygeist-opt/openmpopt.mlir) and also users can hand-code in the OpenMP dialect. Hence, we should produce appropriate error messages for incorrect usage.

Got it. If openmp dialect works as one single unit, this is reasonable. I have to say, linking the flang and openmp dialect units reporting this error is not user-friendly. It's more like the problem of flang frontend. Let's make the good-quality flang frontend to skip this. Anyway, this looks resonable to me now.

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

 LogicalResult AtomicWriteOp::verify() {
   if (auto mo = memory_order_val()) {
     if (*mo == ClauseMemoryOrderKind::Acq_rel ||
         *mo == ClauseMemoryOrderKind::Acquire) {
       return emitError(
           "memory-order must not be acq_rel or acquire for atomic writes");
     }
   }
+  if (address().getType().cast<PointerLikeType>().getElementType() !=
+      value().getType())
+    return emitError("address must dereference to value type");
   return verifySynchronizationHint(*this, hint_val());
 }
 
 //===----------------------------------------------------------------------===//
 // Verifier for AtomicUpdateOp
 //===----------------------------------------------------------------------===//
 
 LogicalResult AtomicUpdateOp::verify() {

mlir/test/Dialect/OpenMP/invalid.mlir

 func.func @omp_atomic_write6(%addr : memref<i32>, %val : i32) {
   // expected-error @below {{invalid clause value: 'xyz'}}
   omp.atomic.write  %addr = %val memory_order(xyz) : memref<i32>, i32
   return
 }
 
 // -----
 
+func.func @omp_atomic_write(%addr : memref<memref<i32>>, %val : i32) {
+  // expected-error @below {{address must dereference to value type}}
+  omp.atomic.write %addr = %val : memref<memref<i32>>, i32
+  return
+}
+
+// -----
+
 func.func @omp_atomic_update1(%x: memref<i32>, %expr: f32) {
   // expected-error @below {{the type of the operand must be a pointer type whose element type is the same as that of the region argument}}
   omp.atomic.update %x : memref<i32> {
   ^bb0(%xval: f32):
     %newval = llvm.fadd %xval, %expr : f32
     omp.yield (%newval : f32)
   }
   return