Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flang] Refine checks for intrinsic operator conflicts with CUDA defi… #94389

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

klausler
Copy link
Contributor

@klausler klausler commented Jun 4, 2024

…ned operators

The checks for conflicts between defined operators/assignments and the intrinsic operators/assignment need to take CUDA procedure and data attributes into account to avoid false positive error messages.

@klausler klausler requested review from clementval and wangzpgi June 4, 2024 19:28
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:semantics labels Jun 4, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 4, 2024

@llvm/pr-subscribers-flang-semantics

Author: Peter Klausler (klausler)

Changes

…ned operators

The checks for conflicts between defined operators/assignments and the intrinsic operators/assignment need to take CUDA procedure and data attributes into account to avoid false positive error messages.


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

2 Files Affected:

  • (modified) flang/lib/Semantics/check-declarations.cpp (+43-12)
  • (added) flang/test/Semantics/cuf16.cuf (+95)
diff --git a/flang/lib/Semantics/check-declarations.cpp b/flang/lib/Semantics/check-declarations.cpp
index 25de9d4af1ffb..bfb38fa1340ec 100644
--- a/flang/lib/Semantics/check-declarations.cpp
+++ b/flang/lib/Semantics/check-declarations.cpp
@@ -1887,11 +1887,34 @@ void CheckHelper::CheckSpecifics(
   helper.Check(generic.owner());
 }
 
+static bool CUDAHostDeviceDiffer(
+    const Procedure &proc, const DummyDataObject &arg) {
+  auto procCUDA{
+      proc.cudaSubprogramAttrs.value_or(common::CUDASubprogramAttrs::Host)};
+  bool procIsHostOnly{procCUDA == common::CUDASubprogramAttrs::Host};
+  bool procIsDeviceOnly{
+      !procIsHostOnly && procCUDA != common::CUDASubprogramAttrs::HostDevice};
+  const auto &argCUDA{arg.cudaDataAttr};
+  bool argIsHostOnly{!argCUDA || *argCUDA == common::CUDADataAttr::Pinned};
+  bool argIsDeviceOnly{(!argCUDA && procIsDeviceOnly) ||
+      (argCUDA &&
+          (*argCUDA != common::CUDADataAttr::Managed &&
+              *argCUDA != common::CUDADataAttr::Pinned &&
+              *argCUDA != common::CUDADataAttr::Unified))};
+  return (procIsHostOnly && argIsDeviceOnly) ||
+      (procIsDeviceOnly && argIsHostOnly);
+}
+
 static bool ConflictsWithIntrinsicAssignment(const Procedure &proc) {
-  auto lhs{std::get<DummyDataObject>(proc.dummyArguments[0].u).type};
-  auto rhs{std::get<DummyDataObject>(proc.dummyArguments[1].u).type};
-  return Tristate::No ==
-      IsDefinedAssignment(lhs.type(), lhs.Rank(), rhs.type(), rhs.Rank());
+  const auto &lhsData{std::get<DummyDataObject>(proc.dummyArguments[0].u)};
+  const auto &lhsTnS{lhsData.type};
+  const auto &rhsData{std::get<DummyDataObject>(proc.dummyArguments[1].u)};
+  const auto &rhsTnS{rhsData.type};
+  return !CUDAHostDeviceDiffer(proc, lhsData) &&
+      !CUDAHostDeviceDiffer(proc, rhsData) &&
+      Tristate::No ==
+      IsDefinedAssignment(
+          lhsTnS.type(), lhsTnS.Rank(), rhsTnS.type(), rhsTnS.Rank());
 }
 
 static bool ConflictsWithIntrinsicOperator(
@@ -1899,8 +1922,12 @@ static bool ConflictsWithIntrinsicOperator(
   if (!kind.IsIntrinsicOperator()) {
     return false;
   }
-  auto arg0{std::get<DummyDataObject>(proc.dummyArguments[0].u).type};
-  auto type0{arg0.type()};
+  const auto &arg0Data{std::get<DummyDataObject>(proc.dummyArguments[0].u)};
+  if (CUDAHostDeviceDiffer(proc, arg0Data)) {
+    return false;
+  }
+  const auto &arg0TnS{arg0Data.type};
+  auto type0{arg0TnS.type()};
   if (proc.dummyArguments.size() == 1) { // unary
     return common::visit(
         common::visitors{
@@ -1910,10 +1937,14 @@ static bool ConflictsWithIntrinsicOperator(
         },
         kind.u);
   } else { // binary
-    int rank0{arg0.Rank()};
-    auto arg1{std::get<DummyDataObject>(proc.dummyArguments[1].u).type};
-    auto type1{arg1.type()};
-    int rank1{arg1.Rank()};
+    int rank0{arg0TnS.Rank()};
+    const auto &arg1Data{std::get<DummyDataObject>(proc.dummyArguments[1].u)};
+    if (CUDAHostDeviceDiffer(proc, arg1Data)) {
+      return false;
+    }
+    const auto &arg1TnS{arg1Data.type};
+    auto type1{arg1TnS.type()};
+    int rank1{arg1TnS.Rank()};
     return common::visit(
         common::visitors{
             [&](common::NumericOperator) {
@@ -2087,8 +2118,8 @@ bool CheckHelper::CheckDefinedAssignment(
     if (!(ok0 && ok1)) {
       return false; // error was reported
     } else if (ConflictsWithIntrinsicAssignment(proc)) {
-      msg = "Defined assignment subroutine '%s' conflicts with"
-            " intrinsic assignment"_err_en_US;
+      msg =
+          "Defined assignment subroutine '%s' conflicts with intrinsic assignment"_err_en_US;
     } else {
       return true; // OK
     }
diff --git a/flang/test/Semantics/cuf16.cuf b/flang/test/Semantics/cuf16.cuf
new file mode 100644
index 0000000000000..4a7595b479503
--- /dev/null
+++ b/flang/test/Semantics/cuf16.cuf
@@ -0,0 +1,95 @@
+! RUN: %python %S/test_errors.py %s %flang_fc1
+module m
+  interface operator(-)
+    !ERROR: OPERATOR(-) function 'f1' conflicts with intrinsic operator
+    function f1(x)
+      real, intent(in) :: x
+    end
+    !ERROR: OPERATOR(-) function 'f2' conflicts with intrinsic operator
+    attributes(device) function f2(x)
+      real, intent(in), device :: x(:)
+    end
+    function f3(x) ! ok
+      real, intent(in), device :: x(:,:)
+    end
+    !ERROR: OPERATOR(-) function 'f4' conflicts with intrinsic operator
+    attributes(device) function f4(x)
+      real, intent(in) :: x(:,:,:)
+    end
+    !ERROR: OPERATOR(-) function 'f5' conflicts with intrinsic operator
+    function f5(x)
+      real, intent(in), unified :: x(:,:,:,:)
+    end
+    !ERROR: OPERATOR(-) function 'f6' conflicts with intrinsic operator
+    attributes(device) function f6(x)
+      real, intent(in), managed :: x(:,:,:,:,:)
+    end
+  end interface
+  interface operator(*)
+    !ERROR: OPERATOR(*) function 'f11' conflicts with intrinsic operator
+    function f11(x, y)
+      real, intent(in) :: x, y
+    end
+    !ERROR: OPERATOR(*) function 'f12' conflicts with intrinsic operator
+    attributes(device) function f12(x, y)
+      real, intent(in), device :: x, y(:)
+    end
+    !ERROR: OPERATOR(*) function 'f13' conflicts with intrinsic operator
+    attributes(device) function f13(x, y)
+      real, intent(in) :: x(:), y
+    end
+    function f14a(x, y) ! ok
+      real, intent(in), device :: x(:)
+      real, intent(in) :: y(:)
+    end
+    function f14b(x, y) ! ok
+      real, intent(in) :: x
+      real, intent(in), device :: y(:,:)
+    end
+    !ERROR: OPERATOR(*) function 'f15' conflicts with intrinsic operator
+    function f15(x, y)
+      real, intent(in) :: x(:,:)
+      real, intent(in), unified :: y
+    end
+    !ERROR: OPERATOR(*) function 'f16' conflicts with intrinsic operator
+    attributes(device) function f16(x, y)
+      real, intent(in), device :: x(:,:)
+      real, intent(in), managed :: y(:,:)
+    end
+  end interface
+  interface assignment(=)
+    !ERROR: Defined assignment subroutine 's1' conflicts with intrinsic assignment
+    subroutine s1(x, y)
+      real, intent(in out) :: x
+      real, intent(in) :: y
+    end
+    !ERROR: Defined assignment subroutine 's2' conflicts with intrinsic assignment
+    attributes(device) subroutine s2(x, y)
+      real, intent(in out), device :: x(:)
+      real, intent(in), device :: y
+    end
+    !ERROR: Defined assignment subroutine 's3' conflicts with intrinsic assignment
+    attributes(device) subroutine s3(x, y)
+      real, intent(in out) :: x(:)
+      real, intent(in) :: y(:)
+    end
+    subroutine s4a(x, y) ! ok
+      real, intent(in out), device :: x(:,:)
+      real, intent(in) :: y
+    end
+    subroutine s4b(x, y) ! ok
+      real, intent(in out) :: x(:,:)
+      real, intent(in), device :: y(:,:)
+    end
+    !ERROR: Defined assignment subroutine 's5' conflicts with intrinsic assignment
+    subroutine s5(x, y)
+      real, intent(in out) :: x(:,:,:)
+      real, intent(in), unified :: y
+    end
+    !ERROR: Defined assignment subroutine 's6' conflicts with intrinsic assignment
+    attributes(device) subroutine s6(x, y)
+      real, intent(in out), device :: x(:,:,:)
+      real, intent(in), managed :: y(:,:,:)
+    end
+  end interface
+end

@klausler
Copy link
Contributor Author

klausler commented Jun 4, 2024

Valentin, Zhen: please check the code and tests carefully to ensure that this change is sound.

@clementval
Copy link
Contributor

Valentin, Zhen: please check the code and tests carefully to ensure that this change is sound.

Valentin, Zhen: please check the code and tests carefully to ensure that this change is sound.

Thanks Peter! I'll have a deeper look and test it later this week. I need to familiarize myself with the issue that lead to the change.

@wangzpgi
Copy link
Contributor

wangzpgi commented Jun 5, 2024

Does your change covers all the following?
operator(*)
operator(+)
operator(-)
operator(.EQ.)
operator(.NE.)
operator(.GT.)
operator(.LT.)
operator(.GE.)
operator(.LE.)
operator(.EQ.)
operator(.NE.)

@klausler
Copy link
Contributor Author

klausler commented Jun 5, 2024

Does your change covers all the following? operator(*) operator(+) operator(-) operator(.EQ.) operator(.NE.) operator(.GT.) operator(.LT.) operator(.GE.) operator(.LE.) operator(.EQ.) operator(.NE.)

Yes, it is independent of the specific operator.

@wangzpgi
Copy link
Contributor

wangzpgi commented Jun 5, 2024

Does your change covers all the following? operator(*) operator(+) operator(-) operator(.EQ.) operator(.NE.) operator(.GT.) operator(.LT.) operator(.GE.) operator(.LE.) operator(.EQ.) operator(.NE.)

Yes, it is independent of the specific operator.

Thanks. This looks good to me. I will let Valentin take another look.

…ned operators

The checks for conflicts between defined operators/assignments and the
intrinsic operators/assignment need to take CUDA procedure and data
attributes into account to avoid false positive error messages.
Copy link
Contributor

@clementval clementval left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@klausler klausler merged commit 0ee7112 into llvm:main Jun 11, 2024
7 checks passed
@klausler klausler deleted the bug1627 branch June 11, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:semantics flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants