[flang][OpenMP] Add TODO messages for partially implemented atomic types #99817

tblah · 2024-07-21T17:27:09Z

There is ongoing work for fir.complex but this looks unlikely to land before the LLVM branch. Adding a TODO message gives cleaner output to users. Currently fir.complex leads to a compiler assertion failure.

Some uses of character types lead to invalid fir.convert type conversions, others make it far enough to hit the same assertion failure as for fir.complex.

There is ongoing work for fir.complex but this looks unlikely to land before the LLVM branch. Adding a TODO message gives cleaner output to users. Currently fir.complex leads to a compiler assertion failure. Some uses of character types lead to invalid fir.convert type conversions, others make it far enough to hit the same assertion failure as for fir.complex.

llvmbot · 2024-07-21T17:27:38Z

@llvm/pr-subscribers-flang-fir-hlfir

@llvm/pr-subscribers-flang-openmp

Author: Tom Eccles (tblah)

Changes

There is ongoing work for fir.complex but this looks unlikely to land before the LLVM branch. Adding a TODO message gives cleaner output to users. Currently fir.complex leads to a compiler assertion failure.

Some uses of character types lead to invalid fir.convert type conversions, others make it far enough to hit the same assertion failure as for fir.complex.

Full diff: https://github.com/llvm/llvm-project/pull/99817.diff

4 Files Affected:

(modified) flang/lib/Lower/DirectivesCommon.h (+23)
(added) flang/test/Lower/OpenMP/Todo/atomic-character.f90 (+8)
(added) flang/test/Lower/OpenMP/Todo/atomic-complex.f90 (+8)
(modified) flang/test/Lower/OpenMP/atomic-read.f90 (+12-16)

diff --git a/flang/lib/Lower/DirectivesCommon.h b/flang/lib/Lower/DirectivesCommon.h
index e8d6bb591e361..8bd3132aaa3cd 100644
--- a/flang/lib/Lower/DirectivesCommon.h
+++ b/flang/lib/Lower/DirectivesCommon.h
@@ -33,6 +33,7 @@
 #include "flang/Optimizer/Builder/FIRBuilder.h"
 #include "flang/Optimizer/Builder/HLFIRTools.h"
 #include "flang/Optimizer/Builder/Todo.h"
+#include "flang/Optimizer/Dialect/FIRType.h"
 #include "flang/Optimizer/HLFIR/HLFIROps.h"
 #include "flang/Parser/parse-tree.h"
 #include "flang/Semantics/openmp-directive-sets.h"
@@ -135,6 +136,22 @@ static inline void genOmpAtomicHintAndMemoryOrderClauses(
   }
 }
 
+template <typename AtomicListT>
+static void processAtomicTODO(mlir::Type elementType, mlir::Location loc) {
+  if (!elementType)
+    return;
+  if constexpr (std::is_same<AtomicListT,
+                             Fortran::parser::OmpAtomicClauseList>()) {
+    // based on assertion for supported element types in OMPIRBuilder.cpp
+    // createAtomicRead
+    mlir::Type unwrappedEleTy = fir::unwrapRefType(elementType);
+    bool supportedAtomicType =
+        (!fir::isa_complex(unwrappedEleTy) && fir::isa_trivial(unwrappedEleTy));
+    if (!supportedAtomicType)
+      TODO(loc, "Unsupported atomic type");
+  }
+}
+
 /// Used to generate atomic.read operation which is created in existing
 /// location set by builder.
 template <typename AtomicListT>
@@ -147,6 +164,8 @@ static inline void genOmpAccAtomicCaptureStatement(
   // Generate `atomic.read` operation for atomic assigment statements
   fir::FirOpBuilder &firOpBuilder = converter.getFirOpBuilder();
 
+  processAtomicTODO<AtomicListT>(elementType, loc);
+
   if constexpr (std::is_same<AtomicListT,
                              Fortran::parser::OmpAtomicClauseList>()) {
     // If no hint clause is specified, the effect is as if
@@ -183,6 +202,8 @@ static inline void genOmpAccAtomicWriteStatement(
   mlir::Type varType = fir::unwrapRefType(lhsAddr.getType());
   rhsExpr = firOpBuilder.createConvert(loc, varType, rhsExpr);
 
+  processAtomicTODO<AtomicListT>(varType, loc);
+
   if constexpr (std::is_same<AtomicListT,
                              Fortran::parser::OmpAtomicClauseList>()) {
     // If no hint clause is specified, the effect is as if
@@ -323,6 +344,8 @@ static inline void genOmpAccAtomicUpdateStatement(
         currentLocation, lhsAddr);
   }
 
+  processAtomicTODO<AtomicListT>(varType, loc);
+
   llvm::SmallVector<mlir::Type> varTys = {varType};
   llvm::SmallVector<mlir::Location> locs = {currentLocation};
   firOpBuilder.createBlock(&atomicUpdateOp->getRegion(0), {}, varTys, locs);
diff --git a/flang/test/Lower/OpenMP/Todo/atomic-character.f90 b/flang/test/Lower/OpenMP/Todo/atomic-character.f90
new file mode 100644
index 0000000000000..88effa4a2a515
--- /dev/null
+++ b/flang/test/Lower/OpenMP/Todo/atomic-character.f90
@@ -0,0 +1,8 @@
+! RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
+
+! CHECK: not yet implemented: Unsupported atomic type
+subroutine character_atomic
+  character :: l, r
+  !$omp atomic read
+    l = r
+end subroutine
diff --git a/flang/test/Lower/OpenMP/Todo/atomic-complex.f90 b/flang/test/Lower/OpenMP/Todo/atomic-complex.f90
new file mode 100644
index 0000000000000..6d6e4399ee192
--- /dev/null
+++ b/flang/test/Lower/OpenMP/Todo/atomic-complex.f90
@@ -0,0 +1,8 @@
+! RUN: %not_todo_cmd %flang_fc1 -emit-hlfir -fopenmp -o - %s 2>&1 | FileCheck %s
+
+! CHECK: not yet implemented: Unsupported atomic type
+subroutine complex_atomic
+  complex :: l, r
+  !$omp atomic read
+    l = r
+end subroutine
diff --git a/flang/test/Lower/OpenMP/atomic-read.f90 b/flang/test/Lower/OpenMP/atomic-read.f90
index 8c3f37c94975e..9559df171111d 100644
--- a/flang/test/Lower/OpenMP/atomic-read.f90
+++ b/flang/test/Lower/OpenMP/atomic-read.f90
@@ -5,22 +5,18 @@
 ! This test checks the lowering of atomic read
 
 !CHECK: func @_QQmain() attributes {fir.bindc_name = "ompatomic"} {
-!CHECK:    %[[A_C1:.*]] = arith.constant 1 : index
-!CHECK:    %[[A_REF:.*]] = fir.alloca !fir.char<1> {bindc_name = "a", uniq_name = "_QFEa"}
-!CHECK:    %[[A_DECL:.*]]:2 = hlfir.declare %[[A_REF]] typeparams %[[A_C1]] {uniq_name = "_QFEa"} : (!fir.ref<!fir.char<1>>, index) -> (!fir.ref<!fir.char<1>>, !fir.ref<!fir.char<1>>)
-!CHECK:    %[[B_C1:.*]] = arith.constant 1 : index
-!CHECK:    %[[B_REF:.*]] = fir.alloca !fir.char<1> {bindc_name = "b", uniq_name = "_QFEb"}
-!CHECK:    %[[B_DECL:.*]]:2 = hlfir.declare %[[B_REF]] typeparams %[[B_C1]] {uniq_name = "_QFEb"} : (!fir.ref<!fir.char<1>>, index) -> (!fir.ref<!fir.char<1>>, !fir.ref<!fir.char<1>>)
+!CHECK:    %[[A_REF:.*]] = fir.alloca i32 {bindc_name = "a", uniq_name = "_QFEa"}
+!CHECK:    %[[A_DECL:.*]]:2 = hlfir.declare %[[A_REF]] {uniq_name = "_QFEa"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:    %[[B_REF:.*]] = fir.alloca i32 {bindc_name = "b", uniq_name = "_QFEb"}
+!CHECK:    %[[B_DECL:.*]]:2 = hlfir.declare %[[B_REF]] {uniq_name = "_QFEb"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK:    %[[C_REF:.*]] = fir.alloca !fir.logical<4> {bindc_name = "c", uniq_name = "_QFEc"}
 !CHECK:    %[[C_DECL:.*]]:2 = hlfir.declare %[[C_REF]] {uniq_name = "_QFEc"} : (!fir.ref<!fir.logical<4>>) -> (!fir.ref<!fir.logical<4>>, !fir.ref<!fir.logical<4>>)
 !CHECK:    %[[D_REF:.*]] = fir.alloca !fir.logical<4> {bindc_name = "d", uniq_name = "_QFEd"}
 !CHECK:    %[[D_DECL:.*]]:2 = hlfir.declare %[[D_REF]] {uniq_name = "_QFEd"} : (!fir.ref<!fir.logical<4>>) -> (!fir.ref<!fir.logical<4>>, !fir.ref<!fir.logical<4>>)
-!CHECK:    %[[E_C8:.*]] = arith.constant 8 : index
-!CHECK:    %[[E_REF:.*]] = fir.alloca !fir.char<1,8> {bindc_name = "e", uniq_name = "_QFEe"}
-!CHECK:    %[[E_DECL:.*]]:2 = hlfir.declare %[[E_REF]] typeparams %[[E_C8]] {uniq_name = "_QFEe"} : (!fir.ref<!fir.char<1,8>>, index) -> (!fir.ref<!fir.char<1,8>>, !fir.ref<!fir.char<1,8>>)
-!CHECK:    %[[F_C8:.*]] = arith.constant 8 : index
-!CHECK:    %[[F_REF:.*]] = fir.alloca !fir.char<1,8> {bindc_name = "f", uniq_name = "_QFEf"}
-!CHECK:    %[[F_DECL:.*]]:2 = hlfir.declare %[[F_REF]] typeparams %[[F_C8]] {uniq_name = "_QFEf"} : (!fir.ref<!fir.char<1,8>>, index) -> (!fir.ref<!fir.char<1,8>>, !fir.ref<!fir.char<1,8>>)
+!CHECK:    %[[E_REF:.*]] = fir.alloca i32 {bindc_name = "e", uniq_name = "_QFEe"}
+!CHECK:    %[[E_DECL:.*]]:2 = hlfir.declare %[[E_REF]] {uniq_name = "_QFEe"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+!CHECK:    %[[F_REF:.*]] = fir.alloca i32 {bindc_name = "f", uniq_name = "_QFEf"}
+!CHECK:    %[[F_DECL:.*]]:2 = hlfir.declare %[[F_REF]] {uniq_name = "_QFEf"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK:    %[[G_REF:.*]] = fir.alloca f32 {bindc_name = "g", uniq_name = "_QFEg"}
 !CHECK:    %[[G_DECL:.*]]:2 = hlfir.declare %[[G_REF]] {uniq_name = "_QFEg"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
 !CHECK:    %[[H_REF:.*]] = fir.alloca f32 {bindc_name = "h", uniq_name = "_QFEh"}
@@ -30,9 +26,9 @@
 !CHECK:    %[[Y_REF:.*]] = fir.alloca i32 {bindc_name = "y", uniq_name = "_QFEy"}
 !CHECK:    %[[Y_DECL:.*]]:2 = hlfir.declare %[[Y_REF]] {uniq_name = "_QFEy"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
 !CHECK:    omp.atomic.read %[[X_DECL]]#1 = %[[Y_DECL]]#1   memory_order(acquire) hint(uncontended) : !fir.ref<i32>, i32
-!CHECK:    omp.atomic.read %[[A_DECL]]#1 = %[[B_DECL]]#1   memory_order(relaxed) : !fir.ref<!fir.char<1>>, !fir.char<1>
+!CHECK:    omp.atomic.read %[[A_DECL]]#1 = %[[B_DECL]]#1   memory_order(relaxed) : !fir.ref<i32>, i32
 !CHECK:    omp.atomic.read %[[C_DECL]]#1 = %[[D_DECL]]#1   memory_order(seq_cst) hint(contended) : !fir.ref<!fir.logical<4>>, !fir.logical<4>
-!CHECK:    omp.atomic.read %[[E_DECL]]#1 = %[[F_DECL]]#1   hint(speculative) : !fir.ref<!fir.char<1,8>>, !fir.char<1,8>
+!CHECK:    omp.atomic.read %[[E_DECL]]#1 = %[[F_DECL]]#1   hint(speculative) : !fir.ref<i32>, i32
 !CHECK:    omp.atomic.read %[[G_DECL]]#1 = %[[H_DECL]]#1   hint(nonspeculative) : !fir.ref<f32>, f32
 !CHECK:    omp.atomic.read %[[G_DECL]]#1 = %[[H_DECL]]#1   : !fir.ref<f32>, f32
 
@@ -40,9 +36,9 @@ program OmpAtomic
 
     use omp_lib
     integer :: x, y
-    character :: a, b
+    integer :: a, b
     logical :: c, d
-    character(8) :: e, f
+    integer :: e, f
     real g, h
     !$omp atomic acquire read hint(omp_sync_hint_uncontended)
        x = y

kiranchandramohan · 2024-07-22T10:57:44Z

Will this TODO hit for both OpenMP and OpenACC? If so, there might be a problem for OpenACC downstream users since they might have an implementation that works.

kiranchandramohan

LGTM.

flang/lib/Lower/DirectivesCommon.h

NimishMishra

LGTM

variable. It looks to me like the warning is bogus, possibly due to a bug in the compiler I'm using (GCC 9.3.0). But adding a "maybe_unused" clause fixes it and makes my builds clean.

After tblah's update #99817, I'm getting a warning about an unusedvariable. It looks to me like the warning is bogus, possibly due to a bug in the compiler I'm using (GCC 9.3.0). But adding a "maybe_unused" clause fixes it and makes my builds clean.

…pes (#99817) Summary: There is ongoing work for fir.complex but this looks unlikely to land before the LLVM branch. Adding a TODO message gives cleaner output to users. Currently fir.complex leads to a compiler assertion failure. Some uses of character types lead to invalid fir.convert type conversions, others make it far enough to hit the same assertion failure as for fir.complex. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251301

Summary: After tblah's update #99817, I'm getting a warning about an unusedvariable. It looks to me like the warning is bogus, possibly due to a bug in the compiler I'm using (GCC 9.3.0). But adding a "maybe_unused" clause fixes it and makes my builds clean. Test Plan: Reviewers: Subscribers: Tasks: Tags: Differential Revision: https://phabricator.intern.facebook.com/D60251162

tblah requested review from DavidTruby, razvanlupusoru, Meinersbur, kiranchandramohan, Leporacanthicus, harishch4, pawosm-arm and NimishMishra July 21, 2024 17:27

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:openmp labels Jul 21, 2024

Fix comment

91cd72e

kiranchandramohan approved these changes Jul 22, 2024

View reviewed changes

flang/lib/Lower/DirectivesCommon.h Outdated Show resolved Hide resolved

Rename to processOmpAtomicTODO

cacf189

tblah force-pushed the ecclescake/atomic-todos branch from e49a77e to cacf189 Compare July 22, 2024 11:27

pawosm-arm approved these changes Jul 22, 2024

View reviewed changes

NimishMishra approved these changes Jul 22, 2024

View reviewed changes

tblah merged commit aa99192 into llvm:main Jul 22, 2024
7 checks passed

psteinfeld mentioned this pull request Jul 23, 2024

[flang] Fix warning in build #100134

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[flang][OpenMP] Add TODO messages for partially implemented atomic types #99817

[flang][OpenMP] Add TODO messages for partially implemented atomic types #99817

Uh oh!

tblah commented Jul 21, 2024

Uh oh!

llvmbot commented Jul 21, 2024 •

edited

Loading

Uh oh!

kiranchandramohan commented Jul 22, 2024 •

edited

Loading

Uh oh!

kiranchandramohan left a comment

Uh oh!

Uh oh!

NimishMishra left a comment

Uh oh!

Uh oh!

Uh oh!

[flang][OpenMP] Add TODO messages for partially implemented atomic types #99817

[flang][OpenMP] Add TODO messages for partially implemented atomic types #99817

Uh oh!

Conversation

tblah commented Jul 21, 2024

Uh oh!

llvmbot commented Jul 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiranchandramohan commented Jul 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NimishMishra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llvmbot commented Jul 21, 2024 •

edited

Loading

kiranchandramohan commented Jul 22, 2024 •

edited

Loading