[Flang][OpenMP][OpenACC][MLIR] Remove typechecks in atomic write/upda… #85059
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
llvmbot
added
mlir
flang
|
@llvm/pr-subscribers-flang-fir-hlfir @llvm/pr-subscribers-mlir Author: None (harishch4) Changes…te Op verifiers The failures in issues #83144(lhsType : fir.logical<4>, rhsType : i1) and #83722(lhsType : fp64, rhsType : fp32) stem from mismatches between types. However, since we already emit a fir.convert operation, I propose removing these checks. This simplification leverages the existing conversion operation to handle these cases appropriately. Full diff: https://github.com/llvm/llvm-project/pull/85059.diff 6 Files Affected:
diff --git a/flang/test/Lower/OpenMP/atomic-capture.f90 b/flang/test/Lower/OpenMP/atomic-capture.f90
index cde0281dbdc849..4b19428e7b1e47 100644
--- a/flang/test/Lower/OpenMP/atomic-capture.f90
+++ b/flang/test/Lower/OpenMP/atomic-capture.f90
@@ -96,3 +96,32 @@ subroutine pointers_in_atomic_capture()
b = a
!$omp end atomic
end subroutine
+
+!CHECK-LABEL: func.func @_QPreal_to_double() {
+subroutine real_to_double()
+ !CHECK: %[[C_ALLOC:.*]] = fir.alloca f64
+ !CHECK: %[[C_REF:.*]] = fir.alloca f32 {bindc_name = "c", uniq_name = "_QFreal_to_doubleEc"}
+ !CHECK: %[[C_DECL:.*]]:2 = hlfir.declare %[[C_REF]] {uniq_name = "_QFreal_to_doubleEc"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+ !CHECK: %[[C2_ALLOC:.*]] = fir.alloca f64 {bindc_name = "c2", uniq_name = "_QFreal_to_doubleEc2"}
+ !CHECK: %[[C2_DECL:.*]]:2 = hlfir.declare %[[C2_ALLOC]] {uniq_name = "_QFreal_to_doubleEc2"} : (!fir.ref<f64>) -> (!fir.ref<f64>, !fir.ref<f64>)
+ real :: c
+ double precision :: c2
+ !CHECK: %[[C_LOAD:.*]] = fir.load %[[C_DECL]]#0 : !fir.ref<f32>
+ !CHECK: %[[C_CVT:.*]] = fir.convert %[[C_LOAD]] : (f32) -> f64
+ !CHECK: fir.store %[[C_CVT]] to %[[C_ALLOC]] : !fir.ref<f64>
+ !CHECK: %cst = arith.constant 2.000000e+00 : f32
+ !CHECK: omp.atomic.capture {
+ !CHECK: omp.atomic.read %[[C2_DECL]]#1 = %[[C_ALLOC]] : !fir.ref<f64>, f32
+ !CHECK: omp.atomic.update %[[C_ALLOC]] : !fir.ref<f64> {
+ !CHECK: ^bb0(%arg0: f32):
+ !CHECK: %[[RES:.*]] = arith.mulf %cst, %arg0 fastmath<contract> : f32
+ !CHECK: omp.yield(%[[RES]] : f32)
+ !CHECK: }
+ !CHECK: }
+ !CHECK: return
+ !CHECK: }
+ !$omp atomic capture
+ c2 = c
+ c = 2.0 * c
+ !$omp end atomic
+ end
diff --git a/flang/test/Lower/OpenMP/atomic-update.f90 b/flang/test/Lower/OpenMP/atomic-update.f90
index 10da97c68c24a1..eeb93dee85941d 100644
--- a/flang/test/Lower/OpenMP/atomic-update.f90
+++ b/flang/test/Lower/OpenMP/atomic-update.f90
@@ -184,3 +184,24 @@ program OmpAtomicUpdate
w = max(w,x,y,z)
end program OmpAtomicUpdate
+
+!CHECK-DAG: %[[X_REF:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "x", uniq_name = "_QFatomic_update_castingEx"}
+!CHECK-DAG: %[[X_DECL:.*]]:2 = hlfir.declare %[[X_REF]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFatomic_update_castingEx"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) -> (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
+!CHECK-DAG: %[[Z_REF:.*]] = fir.alloca f32 {bindc_name = "z", fir.target, uniq_name = "_QFatomic_update_castingEz"}
+!CHECK-DAG: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z_REF]] {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFatomic_update_castingEz"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+!CHECK-DAG: %[[X_LOAD:.*]] = fir.load %[[X_DECL]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
+!CHECK-DAG: %[[X_ADDR:.*]] = fir.box_addr %[[X_LOAD]] : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
+!CHECK-DAG: %[[Z_LOAD:.*]] = fir.load %[[Z_DECL]]#0 : !fir.ref<f32>
+!CHECK-DAG: omp.atomic.update %[[X_ADDR]] : !fir.ptr<i32> {
+!CHECK-DAG: ^bb0(%[[ARG0:.*]]: i32):
+!CHECK-DAG: %[[ARG0_CVT:.*]] = fir.convert %[[ARG0]] : (i32) -> f32
+!CHECK-DAG: %[[RES:.*]] = arith.addf %[[ARG0_CVT]], %[[Z_LOAD]] fastmath<contract> : f32
+!CHECK-DAG: %[[RES_CVT:.*]] = fir.convert %[[RES]] : (f32) -> i32
+!CHECK-DAG: omp.yield(%[[RES_CVT]] : i32)
+!CHECK-DAG: }
+subroutine atomic_update_casting()
+ integer, pointer :: x
+ real, target :: z
+ !$omp atomic update
+ x = x + z
+end
diff --git a/flang/test/Lower/OpenMP/atomic-write.f90 b/flang/test/Lower/OpenMP/atomic-write.f90
index 119f60c1a92f56..908109fa359870 100644
--- a/flang/test/Lower/OpenMP/atomic-write.f90
+++ b/flang/test/Lower/OpenMP/atomic-write.f90
@@ -71,3 +71,39 @@ subroutine atomic_write_typed_assign
!$omp atomic write
r2 = 0
end subroutine
+
+
+!CHECK-LABEL: func.func @_QPatomic_write_logical() {
+!CHECK: %[[L_REF:.*]] = fir.alloca !fir.logical<4> {bindc_name = "l", uniq_name = "_QFatomic_write_logicalEl"}
+!CHECK: %[[L_DECL:.*]]:2 = hlfir.declare %[[L_REF]] {uniq_name = "_QFatomic_write_logicalEl"} : (!fir.ref<!fir.logical<4>>) -> (!fir.ref<!fir.logical<4>>, !fir.ref<!fir.logical<4>>)
+!CHECK: %false = arith.constant false
+!CHECK: omp.atomic.write %[[L_DECL]]#1 = %false : !fir.ref<!fir.logical<4>>, i1
+!CHECK: return
+!CHECK: }
+
+subroutine atomic_write_logical()
+ logical :: l
+ !$omp atomic write
+ l = .false.
+end
+
+
+!CHECK-DAG: %[[X_REF:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "x", uniq_name = "_QFatomic_write_castingEx"}
+!CHECK-DAG: %[[X_DECL:.*]]:2 = hlfir.declare %[[X_REF]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFatomic_write_castingEx"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) -> (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
+!CHECK-DAG: %[[Y_REF:.*]] = fir.alloca i32 {bindc_name = "y", fir.target, uniq_name = "_QFatomic_write_castingEy"}
+!CHECK-DAG: %[[Y_DECL:.*]]:2 = hlfir.declare %[[Y_REF]] {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFatomic_write_castingEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK-DAG: %[[Z_REF:.*]] = fir.alloca f32 {bindc_name = "z", fir.target, uniq_name = "_QFatomic_write_castingEz"}
+!CHECK-DAG: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z_REF]] {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFatomic_write_castingEz"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+!CHECK-DAG: %[[Z_LOAD:.*]] = fir.load %[[Z_DECL]]#0 : !fir.ref<f32>
+!CHECK-DAG: %[[Z_CVT:.*]] = fir.convert %[[Z_LOAD]] : (f32) -> i32
+!CHECK-DAG: %[[X_LOAD:.*]] = fir.load %[[X_DECL]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
+!CHECK-DAG: %[[X_ADDR:.*]] = fir.box_addr %[[X_LOAD]] : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
+!CHECK-DAG: omp.atomic.write %[[X_ADDR:.*]] = %[[Z_CVT]] : !fir.ptr<i32>, i32
+subroutine atomic_write_casting()
+ integer, pointer :: x
+ integer, target :: y
+ real, target :: z
+ x => y
+ !$omp atomic write
+ x = z
+end
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
index 57063ca763d2f2..f460adffd68f8e 100644
--- a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
@@ -86,9 +86,6 @@ def AtomicWriteOpInterface : OpInterface<"AtomicWriteOpInterface"> {
/*args=*/(ins),
/*methodBody=*/"",
/*defaultImplementation=*/[{
- mlir::Type elementType = $_op.getX().getType().getElementType();
- if (elementType && elementType != $_op.getExpr().getType())
- return $_op.emitError("address must dereference to value type");
return mlir::success();
}]
>,
@@ -197,12 +194,6 @@ def AtomicUpdateOpInterface : OpInterface<"AtomicUpdateOpInterface"> {
if ($_op.getRegion().getNumArguments() != 1)
return $_op.emitError("the region must accept exactly one argument");
- Type elementType = $_op.getX().getType().getElementType();
- if (elementType && elementType != $_op.getRegion().getArgument(0).getType()) {
- return $_op.emitError("the type of the operand must be a pointer type whose "
- "element type is the same as that of the region argument");
- }
-
return mlir::success();
}]
>,
diff --git a/mlir/test/Dialect/OpenACC/invalid.mlir b/mlir/test/Dialect/OpenACC/invalid.mlir
index ec5430420524ce..d7700fcd247178 100644
--- a/mlir/test/Dialect/OpenACC/invalid.mlir
+++ b/mlir/test/Dialect/OpenACC/invalid.mlir
@@ -521,26 +521,6 @@ acc.set
// -----
-func.func @acc_atomic_write(%addr : memref<memref<i32>>, %val : i32) {
- // expected-error @below {{address must dereference to value type}}
- acc.atomic.write %addr = %val : memref<memref<i32>>, i32
- return
-}
-
-// -----
-
-func.func @acc_atomic_update(%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}}
- acc.atomic.update %x : memref<i32> {
- ^bb0(%xval: f32):
- %newval = llvm.fadd %xval, %expr : f32
- acc.yield %newval : f32
- }
- return
-}
-
-// -----
-
func.func @acc_atomic_update(%x: memref<i32>, %expr: i32) {
// expected-error @+2 {{op expects regions to end with 'acc.yield', found 'acc.terminator'}}
// expected-note @below {{in custom textual format, the absence of terminator implies 'acc.yield'}}
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index d9261b89e24e3a..f554dbaaafe154 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -703,26 +703,6 @@ func.func @omp_atomic_write6(%addr : memref<i32>, %val : i32) {
// -----
-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
-}
-
-// -----
-
func.func @omp_atomic_update2(%x: memref<i32>, %expr: i32) {
// expected-error @+2 {{op expects regions to end with 'omp.yield', found 'omp.terminator'}}
// expected-note @below {{in custom textual format, the absence of terminator implies 'omp.yield'}}
|
|
@llvm/pr-subscribers-flang-openmp Author: None (harishch4) Changes…te Op verifiers The failures in issues #83144(lhsType : fir.logical<4>, rhsType : i1) and #83722(lhsType : fp64, rhsType : fp32) stem from mismatches between types. However, since we already emit a fir.convert operation, I propose removing these checks. This simplification leverages the existing conversion operation to handle these cases appropriately. Full diff: https://github.com/llvm/llvm-project/pull/85059.diff 6 Files Affected:
diff --git a/flang/test/Lower/OpenMP/atomic-capture.f90 b/flang/test/Lower/OpenMP/atomic-capture.f90
index cde0281dbdc849..4b19428e7b1e47 100644
--- a/flang/test/Lower/OpenMP/atomic-capture.f90
+++ b/flang/test/Lower/OpenMP/atomic-capture.f90
@@ -96,3 +96,32 @@ subroutine pointers_in_atomic_capture()
b = a
!$omp end atomic
end subroutine
+
+!CHECK-LABEL: func.func @_QPreal_to_double() {
+subroutine real_to_double()
+ !CHECK: %[[C_ALLOC:.*]] = fir.alloca f64
+ !CHECK: %[[C_REF:.*]] = fir.alloca f32 {bindc_name = "c", uniq_name = "_QFreal_to_doubleEc"}
+ !CHECK: %[[C_DECL:.*]]:2 = hlfir.declare %[[C_REF]] {uniq_name = "_QFreal_to_doubleEc"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+ !CHECK: %[[C2_ALLOC:.*]] = fir.alloca f64 {bindc_name = "c2", uniq_name = "_QFreal_to_doubleEc2"}
+ !CHECK: %[[C2_DECL:.*]]:2 = hlfir.declare %[[C2_ALLOC]] {uniq_name = "_QFreal_to_doubleEc2"} : (!fir.ref<f64>) -> (!fir.ref<f64>, !fir.ref<f64>)
+ real :: c
+ double precision :: c2
+ !CHECK: %[[C_LOAD:.*]] = fir.load %[[C_DECL]]#0 : !fir.ref<f32>
+ !CHECK: %[[C_CVT:.*]] = fir.convert %[[C_LOAD]] : (f32) -> f64
+ !CHECK: fir.store %[[C_CVT]] to %[[C_ALLOC]] : !fir.ref<f64>
+ !CHECK: %cst = arith.constant 2.000000e+00 : f32
+ !CHECK: omp.atomic.capture {
+ !CHECK: omp.atomic.read %[[C2_DECL]]#1 = %[[C_ALLOC]] : !fir.ref<f64>, f32
+ !CHECK: omp.atomic.update %[[C_ALLOC]] : !fir.ref<f64> {
+ !CHECK: ^bb0(%arg0: f32):
+ !CHECK: %[[RES:.*]] = arith.mulf %cst, %arg0 fastmath<contract> : f32
+ !CHECK: omp.yield(%[[RES]] : f32)
+ !CHECK: }
+ !CHECK: }
+ !CHECK: return
+ !CHECK: }
+ !$omp atomic capture
+ c2 = c
+ c = 2.0 * c
+ !$omp end atomic
+ end
diff --git a/flang/test/Lower/OpenMP/atomic-update.f90 b/flang/test/Lower/OpenMP/atomic-update.f90
index 10da97c68c24a1..eeb93dee85941d 100644
--- a/flang/test/Lower/OpenMP/atomic-update.f90
+++ b/flang/test/Lower/OpenMP/atomic-update.f90
@@ -184,3 +184,24 @@ program OmpAtomicUpdate
w = max(w,x,y,z)
end program OmpAtomicUpdate
+
+!CHECK-DAG: %[[X_REF:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "x", uniq_name = "_QFatomic_update_castingEx"}
+!CHECK-DAG: %[[X_DECL:.*]]:2 = hlfir.declare %[[X_REF]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFatomic_update_castingEx"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) -> (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
+!CHECK-DAG: %[[Z_REF:.*]] = fir.alloca f32 {bindc_name = "z", fir.target, uniq_name = "_QFatomic_update_castingEz"}
+!CHECK-DAG: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z_REF]] {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFatomic_update_castingEz"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+!CHECK-DAG: %[[X_LOAD:.*]] = fir.load %[[X_DECL]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
+!CHECK-DAG: %[[X_ADDR:.*]] = fir.box_addr %[[X_LOAD]] : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
+!CHECK-DAG: %[[Z_LOAD:.*]] = fir.load %[[Z_DECL]]#0 : !fir.ref<f32>
+!CHECK-DAG: omp.atomic.update %[[X_ADDR]] : !fir.ptr<i32> {
+!CHECK-DAG: ^bb0(%[[ARG0:.*]]: i32):
+!CHECK-DAG: %[[ARG0_CVT:.*]] = fir.convert %[[ARG0]] : (i32) -> f32
+!CHECK-DAG: %[[RES:.*]] = arith.addf %[[ARG0_CVT]], %[[Z_LOAD]] fastmath<contract> : f32
+!CHECK-DAG: %[[RES_CVT:.*]] = fir.convert %[[RES]] : (f32) -> i32
+!CHECK-DAG: omp.yield(%[[RES_CVT]] : i32)
+!CHECK-DAG: }
+subroutine atomic_update_casting()
+ integer, pointer :: x
+ real, target :: z
+ !$omp atomic update
+ x = x + z
+end
diff --git a/flang/test/Lower/OpenMP/atomic-write.f90 b/flang/test/Lower/OpenMP/atomic-write.f90
index 119f60c1a92f56..908109fa359870 100644
--- a/flang/test/Lower/OpenMP/atomic-write.f90
+++ b/flang/test/Lower/OpenMP/atomic-write.f90
@@ -71,3 +71,39 @@ subroutine atomic_write_typed_assign
!$omp atomic write
r2 = 0
end subroutine
+
+
+!CHECK-LABEL: func.func @_QPatomic_write_logical() {
+!CHECK: %[[L_REF:.*]] = fir.alloca !fir.logical<4> {bindc_name = "l", uniq_name = "_QFatomic_write_logicalEl"}
+!CHECK: %[[L_DECL:.*]]:2 = hlfir.declare %[[L_REF]] {uniq_name = "_QFatomic_write_logicalEl"} : (!fir.ref<!fir.logical<4>>) -> (!fir.ref<!fir.logical<4>>, !fir.ref<!fir.logical<4>>)
+!CHECK: %false = arith.constant false
+!CHECK: omp.atomic.write %[[L_DECL]]#1 = %false : !fir.ref<!fir.logical<4>>, i1
+!CHECK: return
+!CHECK: }
+
+subroutine atomic_write_logical()
+ logical :: l
+ !$omp atomic write
+ l = .false.
+end
+
+
+!CHECK-DAG: %[[X_REF:.*]] = fir.alloca !fir.box<!fir.ptr<i32>> {bindc_name = "x", uniq_name = "_QFatomic_write_castingEx"}
+!CHECK-DAG: %[[X_DECL:.*]]:2 = hlfir.declare %[[X_REF]] {fortran_attrs = #fir.var_attrs<pointer>, uniq_name = "_QFatomic_write_castingEx"} : (!fir.ref<!fir.box<!fir.ptr<i32>>>) -> (!fir.ref<!fir.box<!fir.ptr<i32>>>, !fir.ref<!fir.box<!fir.ptr<i32>>>)
+!CHECK-DAG: %[[Y_REF:.*]] = fir.alloca i32 {bindc_name = "y", fir.target, uniq_name = "_QFatomic_write_castingEy"}
+!CHECK-DAG: %[[Y_DECL:.*]]:2 = hlfir.declare %[[Y_REF]] {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFatomic_write_castingEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK-DAG: %[[Z_REF:.*]] = fir.alloca f32 {bindc_name = "z", fir.target, uniq_name = "_QFatomic_write_castingEz"}
+!CHECK-DAG: %[[Z_DECL:.*]]:2 = hlfir.declare %[[Z_REF]] {fortran_attrs = #fir.var_attrs<target>, uniq_name = "_QFatomic_write_castingEz"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+!CHECK-DAG: %[[Z_LOAD:.*]] = fir.load %[[Z_DECL]]#0 : !fir.ref<f32>
+!CHECK-DAG: %[[Z_CVT:.*]] = fir.convert %[[Z_LOAD]] : (f32) -> i32
+!CHECK-DAG: %[[X_LOAD:.*]] = fir.load %[[X_DECL]]#0 : !fir.ref<!fir.box<!fir.ptr<i32>>>
+!CHECK-DAG: %[[X_ADDR:.*]] = fir.box_addr %[[X_LOAD]] : (!fir.box<!fir.ptr<i32>>) -> !fir.ptr<i32>
+!CHECK-DAG: omp.atomic.write %[[X_ADDR:.*]] = %[[Z_CVT]] : !fir.ptr<i32>, i32
+subroutine atomic_write_casting()
+ integer, pointer :: x
+ integer, target :: y
+ real, target :: z
+ x => y
+ !$omp atomic write
+ x = z
+end
diff --git a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
index 57063ca763d2f2..f460adffd68f8e 100644
--- a/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
+++ b/mlir/include/mlir/Dialect/OpenACCMPCommon/Interfaces/AtomicInterfaces.td
@@ -86,9 +86,6 @@ def AtomicWriteOpInterface : OpInterface<"AtomicWriteOpInterface"> {
/*args=*/(ins),
/*methodBody=*/"",
/*defaultImplementation=*/[{
- mlir::Type elementType = $_op.getX().getType().getElementType();
- if (elementType && elementType != $_op.getExpr().getType())
- return $_op.emitError("address must dereference to value type");
return mlir::success();
}]
>,
@@ -197,12 +194,6 @@ def AtomicUpdateOpInterface : OpInterface<"AtomicUpdateOpInterface"> {
if ($_op.getRegion().getNumArguments() != 1)
return $_op.emitError("the region must accept exactly one argument");
- Type elementType = $_op.getX().getType().getElementType();
- if (elementType && elementType != $_op.getRegion().getArgument(0).getType()) {
- return $_op.emitError("the type of the operand must be a pointer type whose "
- "element type is the same as that of the region argument");
- }
-
return mlir::success();
}]
>,
diff --git a/mlir/test/Dialect/OpenACC/invalid.mlir b/mlir/test/Dialect/OpenACC/invalid.mlir
index ec5430420524ce..d7700fcd247178 100644
--- a/mlir/test/Dialect/OpenACC/invalid.mlir
+++ b/mlir/test/Dialect/OpenACC/invalid.mlir
@@ -521,26 +521,6 @@ acc.set
// -----
-func.func @acc_atomic_write(%addr : memref<memref<i32>>, %val : i32) {
- // expected-error @below {{address must dereference to value type}}
- acc.atomic.write %addr = %val : memref<memref<i32>>, i32
- return
-}
-
-// -----
-
-func.func @acc_atomic_update(%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}}
- acc.atomic.update %x : memref<i32> {
- ^bb0(%xval: f32):
- %newval = llvm.fadd %xval, %expr : f32
- acc.yield %newval : f32
- }
- return
-}
-
-// -----
-
func.func @acc_atomic_update(%x: memref<i32>, %expr: i32) {
// expected-error @+2 {{op expects regions to end with 'acc.yield', found 'acc.terminator'}}
// expected-note @below {{in custom textual format, the absence of terminator implies 'acc.yield'}}
diff --git a/mlir/test/Dialect/OpenMP/invalid.mlir b/mlir/test/Dialect/OpenMP/invalid.mlir
index d9261b89e24e3a..f554dbaaafe154 100644
--- a/mlir/test/Dialect/OpenMP/invalid.mlir
+++ b/mlir/test/Dialect/OpenMP/invalid.mlir
@@ -703,26 +703,6 @@ func.func @omp_atomic_write6(%addr : memref<i32>, %val : i32) {
// -----
-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
-}
-
-// -----
-
func.func @omp_atomic_update2(%x: memref<i32>, %expr: i32) {
// expected-error @+2 {{op expects regions to end with 'omp.yield', found 'omp.terminator'}}
// expected-note @below {{in custom textual format, the absence of terminator implies 'omp.yield'}}
|
|
@razvanlupusoru @kiranchandramohan @clementval Ping for review. |
When changing MLIR code we should avoid explanations like this since Flang is not the only user of the OpenMP/OpenACC MLIR dialects. Ideally, we should generate the following FIR. Can you check whether we are using the typed or evaluated expression for lowering atomic write? Typed expressions have converts automatically inserted. At the MLIR level, this makes it too permissive. Do we have appropriate errors in the MLIR translation for types that are not compatible? |
|
I am away this week, if others agree then you should feel free to go ahead and submit. |
harishch4
added a commit
that referenced
this pull request
May 17, 2024
Fixes test.f90 in #83144. This issue is observed only when a boolean constant is assigned to a logical variable. In non-openmp flow, a conversion op is inserted before assigning it to a logical variable. This patch will insert a fir.convert operation when the types are not the same, before generating the atomic write operation. I've proposed another patch(#85059 ) which removes checks at MLIR level and looks like it's too permissive. I'm planning to abandon this patch and address it here.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…te Op verifiers
The failures in issues #83144(lhsType : fir.logical<4>, rhsType : i1) and #83722(lhsType : fp64, rhsType : fp32) stem from mismatches between types.
However, since we already emit a fir.convert operation, I propose removing these checks. This simplification leverages the existing conversion operation to handle these cases appropriately.