[Flang] Check if two ArrayConstructor's are Equal #121181

Thirumalai-Shaktivel · 2024-12-27T06:10:11Z

This also includes comparing the two ImpliedDo

Details

For ArrayConstructor, check if x and y have the same
elements and type
For ImpliedDo, check if x and y have the same lower,
upper, stride and values

This also includes comparing the two ImpliedDo Details: - For ArrayConstructor, check if x and y have the same elements and type - For ImpliedDo, check if x and y have the same lower, upper, stride and values

llvmbot · 2024-12-27T06:10:44Z

@llvm/pr-subscribers-flang-openmp

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

Author: Thirumalai Shaktivel (Thirumalai-Shaktivel)

Changes

This also includes comparing the two ImpliedDo

Details

For ArrayConstructor, check if x and y have the same
elements and type
For ImpliedDo, check if x and y have the same lower,
upper, stride and values

Fixes: #104526

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

2 Files Affected:

(modified) flang/include/flang/Lower/Support/Utils.h (+44-1)
(modified) flang/test/Lower/OpenMP/atomic-update.f90 (+15)

diff --git a/flang/include/flang/Lower/Support/Utils.h b/flang/include/flang/Lower/Support/Utils.h
index 1cc74521e22d88..b2f3673f9164cb 100644
--- a/flang/include/flang/Lower/Support/Utils.h
+++ b/flang/include/flang/Lower/Support/Utils.h
@@ -545,9 +545,52 @@ class IsEqualEvaluateExpr {
     return isEqual(x.proc(), y.proc()) && isEqual(x.arguments(), y.arguments());
   }
   template <typename A>
+  static bool isEqual(const Fortran::evaluate::ImpliedDo<A> &x,
+                      const Fortran::evaluate::ImpliedDo<A> &y) {
+    using Expr = Fortran::evaluate::Expr<A>;
+    for (const auto &[xValue, yValue] : llvm::zip(x.values(), y.values())) {
+      bool checkValue = Fortran::common::visit(
+          common::visitors{
+              [&](const Expr &v, const Expr &w) { return isEqual(v, w); },
+              [&](const auto &, const auto &) {
+                llvm::report_fatal_error("isEqual is not handled yet for "
+                    "the element type in ImpliedDo");
+                return false;
+
+              },
+          },
+          xValue.u, yValue.u);
+      if (!checkValue) {
+        return false;
+      }
+    }
+    return isEqual(x.lower(), y.lower()) && isEqual(x.upper(), y.upper()) &&
+           isEqual(x.stride(), y.stride());
+  }
+  template <typename A>
   static bool isEqual(const Fortran::evaluate::ArrayConstructor<A> &x,
                       const Fortran::evaluate::ArrayConstructor<A> &y) {
-    llvm::report_fatal_error("not implemented");
+    for (const auto &[xValue, yValue] : llvm::zip(x, y)) {
+      using Expr = Fortran::evaluate::Expr<A>;
+      using ImpliedDo = Fortran::evaluate::ImpliedDo<A>;
+      bool checkElement = Fortran::common::visit(
+          common::visitors{
+              [&](const Expr &v, const Expr &w) { return isEqual(v, w); },
+              [&](const ImpliedDo &v, const ImpliedDo &w) {
+                return isEqual(v, w);
+              },
+              [&](const auto &, const auto &) {
+                llvm::report_fatal_error("isEqual is not handled yet for "
+                    "the element type in ImpliedDo");
+                return false;
+              },
+          },
+          xValue.u, yValue.u);
+      if (!checkElement) {
+        return false;
+      }
+    }
+    return x.GetType() == y.GetType();
   }
   static bool isEqual(const Fortran::evaluate::ImpliedDoIndex &x,
                       const Fortran::evaluate::ImpliedDoIndex &y) {
diff --git a/flang/test/Lower/OpenMP/atomic-update.f90 b/flang/test/Lower/OpenMP/atomic-update.f90
index 16dae9d5f301c1..7d04745015faab 100644
--- a/flang/test/Lower/OpenMP/atomic-update.f90
+++ b/flang/test/Lower/OpenMP/atomic-update.f90
@@ -185,4 +185,19 @@ program OmpAtomicUpdate
   !$omp atomic update
     w = max(w,x,y,z)
 
+!CHECK:  %[[IMP_DO:.*]] = hlfir.elemental %{{.*}} unordered : (!fir.shape<1>) -> !hlfir.expr<?xi32> {
+!CHECK:  ^bb0(%{{.*}}: index):
+!          [...]
+!CHECK:    %[[ADD_I1:.*]] = arith.addi {{.*}} : i32
+!CHECK:    hlfir.yield_element %[[ADD_I1]] : i32
+!CHECK:  }
+!        [...]
+!CHECK:  %[[SUM:.*]] = hlfir.sum %[[IMP_DO]]
+!CHECK:  omp.atomic.update %[[VAL_X_DECLARE]]#1 : !fir.ref<i32> {
+!CHECK:  ^bb0(%[[ARG0:.*]]: i32):
+!CHECK:    %[[ADD_I2:.*]] = arith.addi %[[ARG0]], %[[SUM]] : i32
+!CHECK:    omp.yield(%[[ADD_I2]] : i32)
+!CHECK:  }
+  !$omp atomic update
+    x = x + sum([ (y+2, y=1, z) ])
 end program OmpAtomicUpdate

github-actions · 2024-12-27T06:14:11Z

✅ With the latest revision this PR passed the C/C++ code formatter.

Thirumalai-Shaktivel · 2025-01-10T09:07:15Z

ping for review
@tblah, @Leporacanthicus, @kiranchandramohan

jeanPerier

Thanks for adding this, few comments inlined.

jeanPerier · 2025-01-10T16:53:35Z

flang/include/flang/Lower/Support/Utils.h

+      bool checkValue = Fortran::common::visit(
+          common::visitors{
+              [&](const Expr &v, const Expr &w) { return isEqual(v, w); },
+              [&](const auto &, const auto &) {
+                llvm::report_fatal_error("isEqual is not handled yet for "
+                                         "the element type in ImpliedDo");
+                return false;
+              },
+          },
+          xValue.u, yValue.u);


I think this the same visitor as the one added below and I would suggest adding and isEqual version for Fortran::evaluate::ArrayConstructorValue<A>.

Then I think you can easily make the coverage complete using the logic below since this is not a "semantic" equality check, but a data structure check, so if one value is an ImpliedDo and the Other is an Expr, the data structure is different (even though the Fortran value of the expression may end-up being the same).

common::visitors{ [&](const Expr &v, const Expr &w) { return isEqual(v, w); }, [&](const ImpliedDo &v, const ImpliedDo &w) { return isEqual(v, w); }, [&](const Expr &, const ImpliedDo &) { return false; }, [&](const ImpliedDo &, const Expr&) { return false; }, }, x.u, y.u);

Note that I suggest using explicit cases instead of auto to avoid silently breaking isEqual in the future if some new member is added to ArrayConstructorValue, even though that is very unlikely (auto would have been fine for an assert/TODO like you were adding).

jeanPerier · 2025-01-10T17:05:35Z

flang/include/flang/Lower/Support/Utils.h

+        return false;
+      }
+    }
+    return x.GetType() == y.GetType();


I think you need a special case for characters for the length_ field that is not reflected in the GetType() (you can put that check under something like if constexpr (A::category == Fortran::common::TypeCategory::Character))

Thirumalai-Shaktivel · 2025-01-21T08:29:40Z

@jeanPerier I have addressed your comments.

jeanPerier

Small nit, LGTM otherwise, thanks for addressing my comments.

jeanPerier · 2025-01-21T10:28:52Z

flang/include/flang/Lower/Support/Utils.h

-    llvm::report_fatal_error("not implemented");
+    bool checkCharacterType = true;
+    if constexpr (A::category == Fortran::common::TypeCategory::Character) {
+      checkCharacterType = x.LEN() == y.LEN();


nit: should use isEqual.

I'm a little confused, sorry about that.

Are you suggesting the following

Suggested change

checkCharacterType = x.LEN() == y.LEN();

checkCharacterType = isEqual(x.LEN(), y.LEN());

or Should I add a isEqual for the Fortran::common::TypeCategory::Character category?

I took the first approach and implemented it: 6442b33, please let me know this works

Thirumalai-Shaktivel · 2025-01-27T08:51:45Z

Thank you for the review!

flang/include/flang/Lower/Support/Utils.h

Thirumalai-Shaktivel · 2025-02-19T08:58:33Z

I will resolve conflicts and merge this PR, thank you, @jeanPerier for the review!

Thirumalai-Shaktivel added 2 commits December 27, 2024 05:21

[Flang] Check if two ArrayConstructor's are Equal

63e1034

This also includes comparing the two ImpliedDo Details: - For ArrayConstructor, check if x and y have the same elements and type - For ImpliedDo, check if x and y have the same lower, upper, stride and values

[Test] Add a test

06b6e83

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

Thirumalai-Shaktivel requested review from tblah, Leporacanthicus, NimishMishra, kiranktp and kaviya2510 December 27, 2024 06:10

Thirumalai-Shaktivel force-pushed the llvm/atomic_01 branch 3 times, most recently from 5588a93 to 06b6e83 Compare December 27, 2024 06:22

Thirumalai-Shaktivel requested a review from kiranchandramohan January 10, 2025 09:06

Thirumalai-Shaktivel mentioned this pull request Jan 10, 2025

[Flang][OpenMP] Important issues to be fixed prior to removing experimental status of OpenMP #110008

Open

75 tasks

tblah requested a review from jeanPerier January 10, 2025 15:21

jeanPerier reviewed Jan 10, 2025

View reviewed changes

Thirumalai-Shaktivel added 5 commits January 16, 2025 09:32

Merge branch 'main' into llvm/atomic_01

8fc48e5

[Flang] Add isEqual visitor for ArrayConstructorValue

0b5e7a8

[Flang] Add check for CharacterType for ArrayConstructor

47acfec

Fix type conflicts

8d7305b

Fix clang format

595c4f6

jeanPerier approved these changes Jan 21, 2025

View reviewed changes

Thirumalai-Shaktivel added 2 commits January 27, 2025 08:42

[Flang] Move the check inside isEqual

6442b33

Merge branch 'main' into llvm/atomic_01

81df952

Thirumalai-Shaktivel commented Jan 27, 2025

View reviewed changes

flang/include/flang/Lower/Support/Utils.h Outdated Show resolved Hide resolved

Fix clang-format

5eae752

Merge branch 'main' into llvm/atomic_01

ddcd61c

Thirumalai-Shaktivel merged commit 160da73 into llvm:main Feb 19, 2025
5 of 7 checks passed

Thirumalai-Shaktivel deleted the llvm/atomic_01 branch February 19, 2025 09:52

joaosaffran mentioned this pull request Feb 26, 2025

[libc++] Set feature-test macro __cpp_lib_atomic_float (#127559) joaosaffran/llvm-project#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flang] Check if two ArrayConstructor's are Equal #121181

[Flang] Check if two ArrayConstructor's are Equal #121181

Thirumalai-Shaktivel commented Dec 27, 2024

llvmbot commented Dec 27, 2024 •

edited

Loading

github-actions bot commented Dec 27, 2024 •

edited

Loading

Thirumalai-Shaktivel commented Jan 10, 2025 •

edited

Loading

jeanPerier left a comment

jeanPerier Jan 10, 2025

jeanPerier Jan 10, 2025

Thirumalai-Shaktivel commented Jan 21, 2025

jeanPerier left a comment

jeanPerier Jan 21, 2025

Thirumalai-Shaktivel Jan 27, 2025

Thirumalai-Shaktivel Jan 27, 2025 •

edited

Loading

Thirumalai-Shaktivel commented Jan 27, 2025

Thirumalai-Shaktivel commented Feb 19, 2025 •

edited

Loading

	checkCharacterType = x.LEN() == y.LEN();
	checkCharacterType = isEqual(x.LEN(), y.LEN());

[Flang] Check if two ArrayConstructor's are Equal #121181

[Flang] Check if two ArrayConstructor's are Equal #121181

Conversation

Thirumalai-Shaktivel commented Dec 27, 2024

llvmbot commented Dec 27, 2024 • edited Loading

github-actions bot commented Dec 27, 2024 • edited Loading

Thirumalai-Shaktivel commented Jan 10, 2025 • edited Loading

jeanPerier left a comment

Choose a reason for hiding this comment

jeanPerier Jan 10, 2025

Choose a reason for hiding this comment

jeanPerier Jan 10, 2025

Choose a reason for hiding this comment

Thirumalai-Shaktivel commented Jan 21, 2025

jeanPerier left a comment

Choose a reason for hiding this comment

jeanPerier Jan 21, 2025

Choose a reason for hiding this comment

Thirumalai-Shaktivel Jan 27, 2025

Choose a reason for hiding this comment

Thirumalai-Shaktivel Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Thirumalai-Shaktivel commented Jan 27, 2025

Thirumalai-Shaktivel commented Feb 19, 2025 • edited Loading

llvmbot commented Dec 27, 2024 •

edited

Loading

github-actions bot commented Dec 27, 2024 •

edited

Loading

Thirumalai-Shaktivel commented Jan 10, 2025 •

edited

Loading

Thirumalai-Shaktivel Jan 27, 2025 •

edited

Loading

Thirumalai-Shaktivel commented Feb 19, 2025 •

edited

Loading