[flang] Add more diagnostic for MAX/MIN intrinsic

[kkwli0]

Summary

Add diagnostic checks for missing mandatory arguments.

https://github.com/llvm/llvm-project/issues/56303

[clementval]

flang/lib/Evaluate/intrinsics.cpp

1465

Why not combining this with CheckMaxMinArgument?

1639

Is there a good reason to use a hard-coded parameter and not directly put it in the string?

1641

Same here.

1644

Same here.

[clementval]

You need to run clang-format on flang/lib/Evaluate/intrinsics.cpp. Pre-commit is failing now.

[klausler]

Thanks for looking into the bugs here and submitting this patch for review. I hope that my comments make sense; please let me know if they don't.

flang/lib/Evaluate/intrinsics.cpp

1470

In the bits of f18 that run before lowering, we consistently use a more modern and strict coding convention that includes braced initialization on declarations for increased safety against inadvertent conversions.

1473

I think that you probably need to test arguments.size() before indexing into it. I'd recommend using .at(0) rather than [0] for extra saftely.

It may be the case that you can elicit these error messages from the general-case code by allowing MAX and MIN to use a two-element actualForDummy array to track A1= and A2= rather than bypassing its use altogether, which is what I think happens now.

1558

Consider putting this code after the loop, before the call to your new function; it confused me a bit on the first reading by being in the loop here. It might also need to handle the case of zero actual arguments, which it won't see here.

1636

Is this special case code for isMaxMin really needed here? Maybe the original general case code can be made to work.

flang/test/Semantics/call23.f90

29

maybe also test A0=, A03= (which would sort before A1), and ABAD=.

[kkwli0]

flang/lib/Evaluate/intrinsics.cpp

1465

As far as I understand CheckMaxMinArgument, it is a per-argument check. For the case of max/min, we need to determine the position of the actual argument in the argument list. If keyword argument is used in the first two actual argument, we need to check all the other arguments till the end (i.e. for the worse case scenario - max(..., a1=x, a2=y)) in order to determine if any error situation is encountered. In my opinion, it is better to wait till all the keyword is collected and then do the check.

1473

Yes, I need to check arguments.size(). I will change [0] to .at(0).

It may be the case that you can elicit these error messages from the general-case code by allowing MAX and MIN to use a two-element actualForDummy array to track A1= and A2= rather than bypassing its use altogether, which is what I think happens now.

As far as I understand, the existing code makes max/min a special case because it can have infinite arguments. The logic for the general case relies on the keywords but max/min does not have in the table if the argument list is larger than 3. I may have missed something here.

1558

Sure. I will move it after the loop for clarity. However, the zero actual arguments case is a different issue (https://github.com/llvm/llvm-project/issues/58921).

1639

Will use dummy[1].keyword instead.

1641

Will use dummy[0].keyword instead.

1644

Will use dummy[0].keyword and dummy[1].keyword instead.

flang/test/Semantics/call23.f90

29

Good point. I will add the variations.

[kkwli0]

Address review comments and rebase.

[clementval]

Thanks for the updates. I'll let @klausler make the final approval.

[klausler]

The debian build has failed; please resolve that failure.

flang/lib/Evaluate/intrinsics.cpp

1605

I think that this would be more clear as a while loop.

1641

You can just say "missing mandatory A2= argument" without needing the string expansion; it would be more clear.

[kkwli0]

flang/lib/Evaluate/intrinsics.cpp

1605

Will do

1641

Will do.

[kkwli0]

Address review comment.

[kkwli0]

In D137742#3931409, @klausler wrote:

The debian build has failed; please resolve that failure.

@klausler May I know what the debian build error is? I am trying to find a debian system to reproduce the problem.

[vzakhari]

In D137742#3932789, @kkwli0 wrote:

In D137742#3931409, @klausler wrote:

The debian build has failed; please resolve that failure.

@klausler May I know what the debian build error is? I am trying to find a debian system to reproduce the problem.

@kkwli0, you may follow the links above in the Diff Detail section. There was a build failure for Diff 2, so if you go to https://reviews.llvm.org/D137742?id=475168, you will see x64 debian failed - and if you click on it you will get to https://buildkite.com/llvm-project/premerge-checks/builds/121619#01847714-2b4a-422f-8da3-f3efd92756d5

Long story short, there was a clang-format issue in flang/lib/Evaluate/intrinsics.cpp and Valentin pointed this out in https://reviews.llvm.org/D137742#3918803 :)

[kkwli0]

Fix format.

Thanks @vzakhari

[kkwli0]

@clementval Do you have more comments? I don't know how to change from the "Needs Review" to "Accepted" state. Thanks.

[clementval]

LGTM

flang/lib/Evaluate/intrinsics.cpp

if (!wasInserted) {
           "Argument keyword '%s=' was repeated in call to '%s'"_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());
+      }
+    }
+  }
+}
+
 static bool CheckAtomicKind(const ActualArgument &arg,
     const semantics::Scope *builtinsScope,
     parser::ContextualMessages &messages) {
   std::string atomicKindStr;
   std::optional<DynamicType> type{arg.GetType()};
 
   if (type->category() == TypeCategory::Integer) {
     atomicKindStr = "atomic_int_kind";
if (!arg) {
       messages.Say(arg->sourceLocation(),
           "alternate return specifier not acceptable on call to intrinsic '%s'"_err_en_US,
           name);
       return std::nullopt;
     } else if (isMaxMin) {
       if (CheckMaxMinArgument(arg->keyword(), maxMinKeywords, name, messages)) {
         actualForDummy.push_back(&*arg);
       } else {
         return std::nullopt;
       }
     } else {
       bool found{false};
       int slot{missingActualArguments};
       for (std::size_t j{0}; j < dummyArgPatterns && !found; ++j) {
         if (dummy[j].optionality == Optionality::missing) {
           continue;
         }
if (!arg) {
           messages.Say(
               "too many actual arguments for intrinsic '%s'"_err_en_US, name);
         }
         return std::nullopt;
       }
     }
   }
 
+  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
   // interface.  Ensure that two or more arguments that have to have the same
   // (or compatible) type and kind do so.  Check for missing non-optional
   // arguments now, too.
   const ActualArgument *sameArg{nullptr};
   const ActualArgument *operandArg{nullptr};
   const IntrinsicDummyArgument *kindDummyArg{nullptr};
   const ActualArgument *kindArg{nullptr};
   bool hasDimArg{false};
   for (std::size_t j{0}; j < dummies; ++j) {
     const IntrinsicDummyArgument &d{dummy[std::min(j, dummyArgPatterns - 1)]};
     if (d.typePattern.kindCode == KindCode::kindArg) {
       CHECK(!kindDummyArg);
       kindDummyArg = &d;
     }
     const ActualArgument *arg{actualForDummy[j]};
     if (!arg) {
       if (d.optionality == Optionality::required) {
-        messages.Say("missing mandatory '%s=' argument"_err_en_US, d.keyword);
+        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().compare(kwA1) == 0) {
+            messages.Say("missing mandatory 'a2=' argument"_err_en_US);
+          } else if (maxMinKeywords.begin()->ToString().compare(kwA2) == 0) {
+            messages.Say("missing mandatory 'a1=' argument"_err_en_US);
+          } else {
+            messages.Say(
+                "missing mandatory 'a1=' and 'a2=' arguments"_err_en_US);
+          }
+        } else {
+          messages.Say(
+              "missing mandatory '%s=' argument"_err_en_US, kw.c_str());
+        }
         return std::nullopt; // missing non-OPTIONAL argument
       } else {
         continue;
       }
     } else if (d.optionality == Optionality::missing) {
       messages.Say(arg->sourceLocation(), "unexpected '%s=' argument"_err_en_US,
           d.keyword);
       return std::nullopt;

flang/test/Semantics/call23.f90

 ! 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 ! prevent folding
+real :: x = 0.0, y = 0.0 , y1 = 0.0 ! prevent folding
 !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
 print *, max(x,a1=1)
 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'
 print *, max(a1=x,a2=0,a06=0)
+!ERROR: missing mandatory 'a2=' argument
+print *, max(a3=y, a1=x)
+!ERROR: missing mandatory 'a1=' argument
+print *, max(a3=y, a2=x)
+!ERROR: missing mandatory 'a1=' and 'a2=' arguments
+print *, max(a3=y, a4=x)
+!ERROR: missing mandatory 'a2=' argument
+print *, max(y1, a3=y)
+!ERROR: missing mandatory 'a1=' and 'a2=' arguments
+print *, max(a9=x, a5=y, a4=y1)
+!ERROR: missing mandatory 'a2=' argument
+print *, max(x)
+!ERROR: missing mandatory 'a2=' argument
+print *, max(a1=x)
+!ERROR: missing mandatory 'a1=' argument
+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'
+print *, max(a0=x)
+!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'
+print *, max(abad=x)
 end