[flang-commits] [flang] [flang] Place MIN/MAX A1/A2 first in semantic analysis (PR #69722)
via flang-commits
flang-commits at lists.llvm.org
Mon Oct 23 00:15:13 PDT 2023
https://github.com/jeanPerier updated https://github.com/llvm/llvm-project/pull/69722
>From e508a6edbd8ac283810a03999db3e5f36af6787e Mon Sep 17 00:00:00 2001
From: Jean Perier <jperier at nvidia.com>
Date: Fri, 20 Oct 2023 06:07:33 -0700
Subject: [PATCH 1/2] [flang] Place MIN/MAX A1/A2 first in semantic analysis
Intrinsic analysis in semantics reorder the actual arguments so that
they match the dummy order. This was not done for MIN/MAX because they
are special: these are the only intrinsics with a variadic number of
arguments.
This caused bugs in lowering that only check the optionality of actual
arguments from the third position (since A1 and A2 are mandatory).
Update semantics to place A1/A2 first. This also allow removing some
checks that were specific to MIN/MAX. There is no point in
sorting/placing the rest of the arguments which would be tedious and
tricky because of the variadic aspect.
---
flang/include/flang/Evaluate/intrinsics.h | 3 +
flang/lib/Evaluate/intrinsics.cpp | 135 ++++++++++------------
flang/test/Lower/Intrinsics/min.f90 | 34 ++++++
flang/test/Semantics/call23.f90 | 22 ++--
4 files changed, 115 insertions(+), 79 deletions(-)
create mode 100644 flang/test/Lower/Intrinsics/min.f90
diff --git a/flang/include/flang/Evaluate/intrinsics.h b/flang/include/flang/Evaluate/intrinsics.h
index 5e06fcba389b278..15afb772ae767b8 100644
--- a/flang/include/flang/Evaluate/intrinsics.h
+++ b/flang/include/flang/Evaluate/intrinsics.h
@@ -99,6 +99,9 @@ class IntrinsicProcTable {
// On success, the actual arguments are transferred to the result
// in dummy argument order; on failure, the actual arguments remain
// untouched.
+ // For MIN and MAX, only a1 and a2 actual arguments are transferred in dummy
+ // order on success and the other arguments are transferred afterwards
+ // without being sorted.
std::optional<SpecificCall> Probe(
const CallCharacteristics &, ActualArguments &, FoldingContext &) const;
diff --git a/flang/lib/Evaluate/intrinsics.cpp b/flang/lib/Evaluate/intrinsics.cpp
index 19cb556c2380c62..cf1008af116ab62 100644
--- a/flang/lib/Evaluate/intrinsics.cpp
+++ b/flang/lib/Evaluate/intrinsics.cpp
@@ -1446,63 +1446,76 @@ static std::int64_t GetBuiltinKind(
// Ensure that the keywords of arguments to MAX/MIN and their variants
// are of the form A123 with no duplicates or leading zeroes.
-static bool CheckMaxMinArgument(std::optional<parser::CharBlock> keyword,
+static bool CheckMaxMinArgument(parser::CharBlock keyword,
std::set<parser::CharBlock> &set, const char *intrinsicName,
parser::ContextualMessages &messages) {
- if (keyword) {
- std::size_t j{1};
- for (; j < keyword->size(); ++j) {
- char ch{(*keyword)[j]};
- if (ch < (j == 1 ? '1' : '0') || ch > '9') {
- break;
- }
+ std::size_t j{1};
+ for (; j < keyword.size(); ++j) {
+ char ch{(keyword)[j]};
+ if (ch < (j == 1 ? '1' : '0') || ch > '9') {
+ break;
}
- if (keyword->size() < 2 || (*keyword)[0] != 'a' || j < keyword->size()) {
- messages.Say(*keyword,
- "Argument keyword '%s=' is not known in call to '%s'"_err_en_US,
- *keyword, intrinsicName);
+ }
+ if (keyword.size() < 2 || (keyword)[0] != 'a' || j < keyword.size()) {
+ messages.Say(keyword,
+ "argument keyword '%s=' is not known in call to '%s'"_err_en_US,
+ keyword, intrinsicName);
+ return false;
+ }
+ auto [_, wasInserted]{set.insert(keyword)};
+ if (!wasInserted) {
+ messages.Say(keyword,
+ "argument keyword '%s=' was repeated in call to '%s'"_err_en_US,
+ keyword, intrinsicName);
+ return false;
+ }
+ return true;
+}
+
+// Validate the keyword, if any, and ensure that A1 and A2 are always placed in
+// first and second position in actualForDummy. A1 and A2 are special since they
+// are not optional. The rest of the arguments are not sorted, there are no
+// differences between them.
+static bool CheckAndPushMinMaxArgument(ActualArgument &arg,
+ std::vector<ActualArgument *> &actualForDummy,
+ std::set<parser::CharBlock> &set, const char *intrinsicName,
+ parser::ContextualMessages &messages) {
+ if (std::optional<parser::CharBlock> keyword{arg.keyword()}) {
+ if (!CheckMaxMinArgument(*keyword, set, intrinsicName, messages)) {
return false;
}
- auto [_, wasInserted]{set.insert(*keyword)};
- if (!wasInserted) {
+ const bool isA1{*keyword == parser::CharBlock{"a1", 2}};
+ if (isA1 && !actualForDummy[0]) {
+ actualForDummy[0] = &arg;
+ return true;
+ }
+ const bool isA2{*keyword == parser::CharBlock{"a2", 2}};
+ if (isA2 && !actualForDummy[1]) {
+ actualForDummy[1] = &arg;
+ return true;
+ }
+ if (isA1 || isA2) {
+ // Note that for arguments other than a1 and a2, this error will be caught
+ // later in check-call.cpp.
messages.Say(*keyword,
- "Argument keyword '%s=' was repeated in call to '%s'"_err_en_US,
+ "keyword argument '%s=' to intrinsic '%s' was supplied "
+ "positionally by an earlier actual argument"_err_en_US,
*keyword, intrinsicName);
return false;
}
- }
- return true;
-}
-
-static void CheckMaxMinA1A2Argument(const ActualArguments &arguments,
- std::set<parser::CharBlock> &set, parser::ContextualMessages &messages) {
- parser::CharBlock kwA1{"a1", 2};
- parser::CharBlock kwA2{"a2", 2};
- bool missingA1{set.find(kwA1) == set.end()};
- bool missingA2{set.find(kwA2) == set.end()};
-
- if (arguments.size() > 1) {
- if (arguments.at(0)->keyword()) {
- // If the keyword is specified in the first argument, the following
- // arguments must have the keywords.
- if (missingA1 && missingA2) {
- messages.Say("missing mandatory '%s=' and '%s=' arguments"_err_en_US,
- kwA1.ToString(), kwA2.ToString());
- } else if (missingA1 && !missingA2) {
- messages.Say(
- "missing mandatory '%s=' argument"_err_en_US, kwA1.ToString());
- } else if (!missingA1 && missingA2) {
- messages.Say(
- "missing mandatory '%s=' argument"_err_en_US, kwA2.ToString());
- }
- } else if (arguments.at(1)->keyword()) {
- // No keyword is specified in the first argument.
- if (missingA1 && missingA2) {
- messages.Say(
- "missing mandatory '%s=' argument"_err_en_US, kwA2.ToString());
+ } else {
+ if (actualForDummy.size() == 2) {
+ if (!actualForDummy[0] && !actualForDummy[1]) {
+ actualForDummy[0] = &arg;
+ return true;
+ } else if (!actualForDummy[1]) {
+ actualForDummy[1] = &arg;
+ return true;
}
}
}
+ actualForDummy.push_back(&arg);
+ return true;
}
static bool CheckAtomicKind(const ActualArgument &arg,
@@ -1552,7 +1565,7 @@ std::optional<SpecificCall> IntrinsicInterface::Match(
bool isMaxMin{dummyArgPatterns > 0 &&
dummy[dummyArgPatterns - 1].optionality == Optionality::repeats};
std::vector<ActualArgument *> actualForDummy(
- isMaxMin ? 0 : dummyArgPatterns, nullptr);
+ isMaxMin ? 2 : dummyArgPatterns, nullptr);
bool anyMissingActualArgument{false};
std::set<parser::CharBlock> maxMinKeywords;
bool anyKeyword{false};
@@ -1579,9 +1592,8 @@ std::optional<SpecificCall> IntrinsicInterface::Match(
continue;
}
if (isMaxMin) {
- if (CheckMaxMinArgument(arg->keyword(), maxMinKeywords, name, messages)) {
- actualForDummy.push_back(&*arg);
- } else {
+ if (!CheckAndPushMinMaxArgument(
+ *arg, actualForDummy, maxMinKeywords, name, messages)) {
return std::nullopt;
}
} else {
@@ -1627,17 +1639,6 @@ std::optional<SpecificCall> IntrinsicInterface::Match(
}
}
- if (isMaxMin) {
- int nArgs{0};
- // max() / max(x) is invalid
- while ((arguments.size() + nArgs) < 2) {
- actualForDummy.push_back(nullptr);
- nArgs++;
- }
-
- CheckMaxMinA1A2Argument(arguments, maxMinKeywords, messages);
- }
-
std::size_t dummies{actualForDummy.size()};
// Check types and kinds of the actual arguments against the intrinsic's
@@ -1659,18 +1660,8 @@ std::optional<SpecificCall> IntrinsicInterface::Match(
if (!arg) {
if (d.optionality == Optionality::required) {
std::string kw{d.keyword};
- if (isMaxMin && maxMinKeywords.size() == 1) {
- // max(a1=x) or max(a2=x)
- const auto kwA1{dummy[0].keyword};
- const auto kwA2{dummy[1].keyword};
- if (maxMinKeywords.begin()->ToString() == kwA1) {
- messages.Say("missing mandatory 'a2=' argument"_err_en_US);
- } else if (maxMinKeywords.begin()->ToString() == kwA2) {
- messages.Say("missing mandatory 'a1=' argument"_err_en_US);
- } else {
- messages.Say(
- "missing mandatory 'a1=' and 'a2=' arguments"_err_en_US);
- }
+ if (isMaxMin && !actualForDummy[0] && !actualForDummy[1]) {
+ messages.Say("missing mandatory 'a1=' and 'a2=' arguments"_err_en_US);
} else {
messages.Say(
"missing mandatory '%s=' argument"_err_en_US, kw.c_str());
diff --git a/flang/test/Lower/Intrinsics/min.f90 b/flang/test/Lower/Intrinsics/min.f90
new file mode 100644
index 000000000000000..40e6d96db6e53e7
--- /dev/null
+++ b/flang/test/Lower/Intrinsics/min.f90
@@ -0,0 +1,34 @@
+! RUN: bbc -emit-hlfir -o - %s 2>&1 | FileCheck %s
+! Test that min/max A(X>2) optional arguments are handled regardless
+! of the order in which they appear. Only A1 and A2 are mandatory.
+
+real function test(a, b, c)
+ real, optional :: a, b, c
+ test = min(a1=a, a3=c, a2=c)
+end function
+! CHECK-LABEL: func.func @_QPtest(
+! CHECK-SAME: %[[VAL_0:.*]]: !fir.ref<f32> {fir.bindc_name = "a", fir.optional},
+! CHECK-SAME: %[[VAL_1:.*]]: !fir.ref<f32> {fir.bindc_name = "b", fir.optional},
+! CHECK-SAME: %[[VAL_2:.*]]: !fir.ref<f32> {fir.bindc_name = "c", fir.optional}) -> f32 {
+! CHECK: %[[VAL_3:.*]]:2 = hlfir.declare %[[VAL_0]] {{.*}}uniq_name = "_QFtestEa"}
+! CHECK: %[[VAL_4:.*]]:2 = hlfir.declare %[[VAL_1]] {{.*}}uniq_name = "_QFtestEb"}
+! CHECK: %[[VAL_5:.*]]:2 = hlfir.declare %[[VAL_2]] {{.*}}uniq_name = "_QFtestEc"}
+! CHECK: %[[VAL_6:.*]] = fir.alloca f32 {bindc_name = "test", uniq_name = "_QFtestEtest"}
+! CHECK: %[[VAL_7:.*]]:2 = hlfir.declare %[[VAL_6]] {uniq_name = "_QFtestEtest"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
+! CHECK: %[[VAL_8:.*]] = fir.load %[[VAL_3]]#0 : !fir.ref<f32>
+! CHECK: %[[VAL_9:.*]] = fir.load %[[VAL_5]]#0 : !fir.ref<f32>
+! CHECK: %[[VAL_10:.*]] = fir.is_present %[[VAL_5]]#0 : (!fir.ref<f32>) -> i1
+! CHECK: %[[VAL_11:.*]] = arith.cmpf olt, %[[VAL_8]], %[[VAL_9]] : f32
+! CHECK: %[[VAL_12:.*]] = arith.select %[[VAL_11]], %[[VAL_8]], %[[VAL_9]] : f32
+! CHECK: %[[VAL_13:.*]] = fir.if %[[VAL_10]] -> (f32) {
+! CHECK: %[[VAL_14:.*]] = fir.load %[[VAL_5]]#0 : !fir.ref<f32>
+! CHECK: %[[VAL_15:.*]] = arith.cmpf olt, %[[VAL_12]], %[[VAL_14]] : f32
+! CHECK: %[[VAL_16:.*]] = arith.select %[[VAL_15]], %[[VAL_12]], %[[VAL_14]] : f32
+! CHECK: fir.result %[[VAL_16]] : f32
+! CHECK: } else {
+! CHECK: fir.result %[[VAL_12]] : f32
+! CHECK: }
+! CHECK: hlfir.assign %[[VAL_13]] to %[[VAL_7]]#0 : f32, !fir.ref<f32>
+! CHECK: %[[VAL_17:.*]] = fir.load %[[VAL_7]]#1 : !fir.ref<f32>
+! CHECK: return %[[VAL_17]] : f32
+! CHECK: }
diff --git a/flang/test/Semantics/call23.f90 b/flang/test/Semantics/call23.f90
index 38c860ad3b51670..e5c205dba885e5f 100644
--- a/flang/test/Semantics/call23.f90
+++ b/flang/test/Semantics/call23.f90
@@ -1,15 +1,15 @@
! RUN: %python %S/test_errors.py %s %flang_fc1
! Check errors on MAX/MIN with keywords, a weird case in Fortran
real :: x = 0.0, y = 0.0 , y1 = 0.0 ! prevent folding
-!ERROR: Argument keyword 'a1=' was repeated in call to 'max'
+!ERROR: argument keyword 'a1=' was repeated in call to 'max'
print *, max(a1=x,a1=1)
-!ERROR: Keyword argument 'a1=' has already been specified positionally (#1) in this procedure reference
+!ERROR: keyword argument 'a1=' to intrinsic 'max' was supplied positionally by an earlier actual argument
print *, max(x,a1=1)
-!ERROR: Keyword argument 'a1=' has already been specified positionally (#1) in this procedure reference
+!ERROR: keyword argument 'a1=' to intrinsic 'min1' was supplied positionally by an earlier actual argument
print *, min1(1.,a1=2.,a2=3.)
print *, max(a1=x,a2=0,a4=0) ! ok
print *, max(x,0,a99=0) ! ok
-!ERROR: Argument keyword 'a06=' is not known in call to 'max'
+!ERROR: argument keyword 'a06=' is not known in call to 'max'
print *, max(a1=x,a2=0,a06=0)
!ERROR: missing mandatory 'a2=' argument
print *, max(a3=y, a1=x)
@@ -29,10 +29,18 @@
print *, max(a2=y)
!ERROR: missing mandatory 'a1=' and 'a2=' arguments
print *, max(a3=x)
-!ERROR: Argument keyword 'a0=' is not known in call to 'max'
+!ERROR: argument keyword 'a0=' is not known in call to 'max'
print *, max(a0=x)
-!ERROR: Argument keyword 'a03=' is not known in call to 'max'
+!ERROR: argument keyword 'a03=' is not known in call to 'max'
print *, max(a03=x)
-!ERROR: Argument keyword 'abad=' is not known in call to 'max'
+!ERROR: argument keyword 'abad=' is not known in call to 'max'
print *, max(abad=x)
+!ERROR: actual argument #2 without a keyword may not follow an actual argument with a keyword
+print *, max(a1=x, y)
+!ERROR: Keyword argument 'a3=' has already been specified positionally (#3) in this procedure reference
+print *, max(x, y, y1, a3=x)
+print *, max(a3=x, a4=x, a2=x, a1=x) ! ok
+print *, max(a3=x, a2=y, a4=x, a1=x) ! ok
+print *, max(x, a2=y, a5=x, a4=x) ! ok
+print *, max(x, y, a4=x, a6=x) ! ok
end
>From 8a08e0e78421564cff274818e72fb3f50b9e1975 Mon Sep 17 00:00:00 2001
From: Jean Perier <jperier at nvidia.com>
Date: Mon, 23 Oct 2023 00:14:07 -0700
Subject: [PATCH 2/2] address comment: remove wasInserted
---
flang/lib/Evaluate/intrinsics.cpp | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/flang/lib/Evaluate/intrinsics.cpp b/flang/lib/Evaluate/intrinsics.cpp
index cf1008af116ab62..e4125f6572aa983 100644
--- a/flang/lib/Evaluate/intrinsics.cpp
+++ b/flang/lib/Evaluate/intrinsics.cpp
@@ -1462,8 +1462,7 @@ static bool CheckMaxMinArgument(parser::CharBlock keyword,
keyword, intrinsicName);
return false;
}
- auto [_, wasInserted]{set.insert(keyword)};
- if (!wasInserted) {
+ if (!set.insert(keyword).second) {
messages.Say(keyword,
"argument keyword '%s=' was repeated in call to '%s'"_err_en_US,
keyword, intrinsicName);
More information about the flang-commits
mailing list