[flang] Fix for atand(Y,X), and implment atan2d(Y,X), atanpi(X), atanpi(Y,X), atan2pi(Y,X) #79002

yi-wu-arm · 2024-01-22T16:10:03Z

Both call the mlir::math::Atan2op, with the non zero check of: if Y is zeroX must not be 0.
Fix: #78568

llvmbot · 2024-01-22T16:10:38Z

@llvm/pr-subscribers-flang-semantics

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

Author: Yi Wu (yi-wu-arm)

Changes

Both call the mlir::math::Atan2op
However, based on the new (working draft) standard 2023, there are some differences between these two intrinsics:
atand(y,x): no check for 0
atan2d(y,x): when y==0, x must not be 0
Fix: #78568

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

6 Files Affected:

(modified) flang/include/flang/Optimizer/Builder/IntrinsicCall.h (+1)
(modified) flang/lib/Evaluate/intrinsics.cpp (+7-3)
(modified) flang/lib/Optimizer/Builder/IntrinsicCall.cpp (+45-4)
(added) flang/test/Lower/Intrinsics/atan2d.f90 (+28)
(added) flang/test/Lower/Intrinsics/atand-optional.f90 (+14)
(modified) flang/test/Lower/Intrinsics/atand.f90 (+21-3)

diff --git a/flang/include/flang/Optimizer/Builder/IntrinsicCall.h b/flang/include/flang/Optimizer/Builder/IntrinsicCall.h
index 80f79d42fc2b75c..e85ad402fb2c33c 100644
--- a/flang/include/flang/Optimizer/Builder/IntrinsicCall.h
+++ b/flang/include/flang/Optimizer/Builder/IntrinsicCall.h
@@ -174,6 +174,7 @@ struct IntrinsicLibrary {
   mlir::Value genAnint(mlir::Type, llvm::ArrayRef<mlir::Value>);
   fir::ExtendedValue genAny(mlir::Type, llvm::ArrayRef<fir::ExtendedValue>);
   mlir::Value genAtand(mlir::Type, llvm::ArrayRef<mlir::Value>);
+  mlir::Value genAtan2d(mlir::Type, llvm::ArrayRef<mlir::Value>);
   fir::ExtendedValue
       genCommandArgumentCount(mlir::Type, llvm::ArrayRef<fir::ExtendedValue>);
   fir::ExtendedValue genAssociated(mlir::Type,
diff --git a/flang/lib/Evaluate/intrinsics.cpp b/flang/lib/Evaluate/intrinsics.cpp
index da6d5970089884c..c059fa3502c35e2 100644
--- a/flang/lib/Evaluate/intrinsics.cpp
+++ b/flang/lib/Evaluate/intrinsics.cpp
@@ -333,11 +333,15 @@ static const IntrinsicInterface genericIntrinsicFunction[]{
                 common::Intent::In, {ArgFlag::canBeNull}}},
         DefaultLogical, Rank::elemental, IntrinsicClass::inquiryFunction},
     {"atan", {{"x", SameFloating}}, SameFloating},
-    {"atand", {{"x", SameFloating}}, SameFloating},
     {"atan", {{"y", OperandReal}, {"x", OperandReal}}, OperandReal},
-    {"atand", {{"y", OperandReal}, {"x", OperandReal}}, OperandReal},
+    {"atand",
+        {{"y", SameReal, Rank::scalar, Optionality::required,
+             common::Intent::In},
+            {"x", SameReal, Rank::scalar, Optionality::optional,
+                common::Intent::In}},
+        SameReal, Rank::scalar, IntrinsicClass::inquiryFunction},
     {"atan2", {{"y", OperandReal}, {"x", OperandReal}}, OperandReal},
-    {"atan2d", {{"y", OperandReal}, {"x", OperandReal}}, OperandReal},
+    {"atan2d", {{"y", SameReal}, {"x", SameReal}}, SameReal},
     {"atanh", {{"x", SameFloating}}, SameFloating},
     {"bessel_j0", {{"x", SameReal}}, SameReal},
     {"bessel_j1", {{"x", SameReal}}, SameReal},
diff --git a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
index ac7d4fbe23e6738..b685878ad8c60f6 100644
--- a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
+++ b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
@@ -138,6 +138,7 @@ static constexpr IntrinsicHandler handlers[]{
      &I::genAssociated,
      {{{"pointer", asInquired}, {"target", asInquired}}},
      /*isElemental=*/false},
+    {"atan2d", &I::genAtan2d},
     {"atand", &I::genAtand},
     {"bessel_jn",
      &I::genBesselJn,
@@ -2128,13 +2129,53 @@ IntrinsicLibrary::genAny(mlir::Type resultType,
   return readAndAddCleanUp(resultMutableBox, resultType, "ANY");
 }
 
+// ATAN2D
+mlir::Value IntrinsicLibrary::genAtan2d(mlir::Type resultType,
+                                        llvm::ArrayRef<mlir::Value> args) {
+  assert(args.size() == 2);
+
+  mlir::Value y = fir::getBase(args[0]);
+  mlir::Value x = fir::getBase(args[1]);
+
+  // When Y == 0 X must not be 0
+  mlir::Value zero = builder.createRealZeroConstant(loc, y.getType());
+  mlir::Value cmpYEq0 = builder.create<mlir::arith::CmpFOp>(
+      loc, mlir::arith::CmpFPredicate::UEQ, y, zero);
+  mlir::Value cmpXEq0 = builder.create<mlir::arith::CmpFOp>(
+      loc, mlir::arith::CmpFPredicate::UEQ, x, zero);
+  mlir::Value terminationCheck =
+      builder.create<mlir::arith::AndIOp>(loc, cmpYEq0, cmpXEq0);
+  builder.genIfThenElse(loc, terminationCheck)
+      .genThen([&]() {
+        fir::runtime::genReportFatalUserError(builder, loc,
+                                              "When Y == 0 X must not be 0");
+      })
+      .end();
+
+  // atand(y,x) atan2d(y,x) == atan2(y,x) * 180/pi
+  mlir::Value atan = builder.create<mlir::math::Atan2Op>(loc, y, x);
+  mlir::MLIRContext *context = builder.getContext();
+  llvm::APFloat pi = llvm::APFloat(llvm::numbers::pi);
+  mlir::Value dfactor = builder.createRealConstant(
+      loc, mlir::FloatType::getF64(context), llvm::APFloat(180.0) / pi);
+  mlir::Value factor = builder.createConvert(loc, resultType, dfactor);
+  return builder.create<mlir::arith::MulFOp>(loc, atan, factor);
+}
+
+// ATAND
 mlir::Value IntrinsicLibrary::genAtand(mlir::Type resultType,
                                        llvm::ArrayRef<mlir::Value> args) {
-  assert(args.size() == 1);
+  assert(args.size() == 2);
+
+  mlir::Value atan;
+
+  // atand(y,x) atan2d(y,x) == atan2(y,x) * 180/pi
+  if (isStaticallyPresent(args[1])) {
+    atan = builder.create<mlir::math::Atan2Op>(loc, args[0], args[1]);
+  } else {
+    atan = builder.create<mlir::math::AtanOp>(loc, args[0]);
+  }
   mlir::MLIRContext *context = builder.getContext();
-  mlir::FunctionType ftype =
-      mlir::FunctionType::get(context, {resultType}, {args[0].getType()});
-  mlir::Value atan = getRuntimeCallGenerator("atan", ftype)(builder, loc, args);
   llvm::APFloat pi = llvm::APFloat(llvm::numbers::pi);
   mlir::Value dfactor = builder.createRealConstant(
       loc, mlir::FloatType::getF64(context), llvm::APFloat(180.0) / pi);
diff --git a/flang/test/Lower/Intrinsics/atan2d.f90 b/flang/test/Lower/Intrinsics/atan2d.f90
new file mode 100644
index 000000000000000..f94c2d79c0d4b79
--- /dev/null
+++ b/flang/test/Lower/Intrinsics/atan2d.f90
@@ -0,0 +1,28 @@
+! RUN: bbc -emit-fir -hlfir=false %s -o - | FileCheck %s --check-prefixes="CHECK,CHECK-FAST"
+! RUN: %flang_fc1 -emit-fir -flang-deprecated-no-hlfir %s -o - | FileCheck %s --check-prefixes="CHECK,CHECK-FAST"
+
+
+function test_real4_all_args(y,x)
+  real(4) :: x, y, test_real4
+  test_real4 = atan2d(y,x)
+end function
+
+! CHECK-LABEL: @_QPtest_real4_all_args
+! CHECK: %[[terminationCheck:.*]] = arith.andi %[[YEq0:.*]], %[[XEq0:.*]] : i1
+! CHECK: fir.if %[[terminationCheck]]
+! CHECK-FAST: %[[atan2:.*]] = math.atan2 %{{.*}}, %{{.*}}: f32
+! CHECK: %[[dfactor:.*]] = arith.constant 57.295779513082323 : f64
+! CHECK: %[[factor:.*]] = fir.convert %[[dfactor]] : (f64) -> f32
+! CHECK: %{{.*}} = arith.mulf %[[atan2]], %[[factor]] fastmath<contract> : f32
+
+function test_real8_all_args(y,x)
+  real(8) :: x, y, test_real8
+  test_real8 = atan2d(y,x)
+end function
+
+! CHECK-LABEL: @_QPtest_real8_all_args
+! CHECK: %[[terminationCheck:.*]] = arith.andi %[[YEq0:.*]], %[[XEq0:.*]] : i1
+! CHECK: fir.if %[[terminationCheck]]
+! CHECK-FAST: %[[atan2:.*]] = math.atan2 %{{.*}}, %{{.*}}: f64
+! CHECK: %[[factor:.*]] = arith.constant 57.295779513082323 : f64
+! CHECK: %{{.*}} = arith.mulf %[[atan2]], %[[factor]] fastmath<contract> : f64
diff --git a/flang/test/Lower/Intrinsics/atand-optional.f90 b/flang/test/Lower/Intrinsics/atand-optional.f90
new file mode 100644
index 000000000000000..b3b98144ead42ee
--- /dev/null
+++ b/flang/test/Lower/Intrinsics/atand-optional.f90
@@ -0,0 +1,14 @@
+! RUN: bbc -emit-fir -hlfir=false %s -o - | FileCheck %s --check-prefixes="CHECK,CHECK-FAST"
+! RUN: %flang_fc1 -emit-fir -flang-deprecated-no-hlfir %s -o - | FileCheck %s --check-prefixes="CHECK,CHECK-FAST"
+
+function test_real4_all_args_optional(y,x)
+  real(4), optional :: x, y
+  real(4) :: test_real4
+  test_real4 = atand(y,x)
+end function
+
+! CHECK-LABEL: @_QPtest_real4_all_args_optional
+! CHECK-FAST: %[[atan2:.*]] = math.atan2 %{{.*}}, %{{.*}}: f32
+! CHECK: %[[dfactor:.*]] = arith.constant 57.295779513082323 : f64
+! CHECK: %[[factor:.*]] = fir.convert %[[dfactor]] : (f64) -> f32
+! CHECK: %{{.*}} = arith.mulf %[[atan2]], %[[factor]] fastmath<contract> : f32
diff --git a/flang/test/Lower/Intrinsics/atand.f90 b/flang/test/Lower/Intrinsics/atand.f90
index 2483bef46e60f6f..25d882b693ed81c 100644
--- a/flang/test/Lower/Intrinsics/atand.f90
+++ b/flang/test/Lower/Intrinsics/atand.f90
@@ -1,5 +1,4 @@
 ! RUN: bbc -emit-fir -hlfir=false %s -o - | FileCheck %s --check-prefixes="CHECK,CHECK-FAST"
-! RUN: bbc --math-runtime=precise -emit-fir -hlfir=false %s -o - | FileCheck %s --check-prefixes="CHECK,CHECK-PRECISE"
 ! RUN: %flang_fc1 -emit-fir -flang-deprecated-no-hlfir %s -o - | FileCheck %s --check-prefixes="CHECK,CHECK-FAST"
 
 function test_real4(x)
@@ -8,7 +7,6 @@ function test_real4(x)
 end function
 
 ! CHECK-LABEL: @_QPtest_real4
-! CHECK-PRECISE: %[[atan:.*]] = fir.call @atanf({{%[A-Za-z0-9._]+}}) fastmath<contract> : (f32) -> f32
 ! CHECK-FAST: %[[atan:.*]] = math.atan %{{.*}} : f32
 ! CHECK: %[[dfactor:.*]] = arith.constant 57.295779513082323 : f64
 ! CHECK: %[[factor:.*]] = fir.convert %[[dfactor]] : (f64) -> f32
@@ -20,7 +18,27 @@ function test_real8(x)
 end function
 
 ! CHECK-LABEL: @_QPtest_real8
-! CHECK-PRECISE: %[[atan:.*]] = fir.call @atan({{%[A-Za-z0-9._]+}}) fastmath<contract> : (f64) -> f64
 ! CHECK-FAST: %[[atan:.*]] = math.atan %{{.*}} : f64
 ! CHECK: %[[factor:.*]] = arith.constant 57.295779513082323 : f64
 ! CHECK: %{{.*}} = arith.mulf %[[atan]], %[[factor]] fastmath<contract> : f64
+
+function test_real4_all_args(y,x)
+  real(4) :: x, y, test_real4
+  test_real4 = atand(y,x)
+end function
+
+! CHECK-LABEL: @_QPtest_real4_all_args
+! CHECK-FAST: %[[atan2:.*]] = math.atan2 %{{.*}}, %{{.*}}: f32
+! CHECK: %[[dfactor:.*]] = arith.constant 57.295779513082323 : f64
+! CHECK: %[[factor:.*]] = fir.convert %[[dfactor]] : (f64) -> f32
+! CHECK: %{{.*}} = arith.mulf %[[atan2]], %[[factor]] fastmath<contract> : f32
+
+function test_real8_all_args(y,x)
+  real(8) :: x, y, test_real8
+  test_real8 = atand(y,x)
+end function
+
+! CHECK-LABEL: @_QPtest_real8_all_args
+! CHECK-FAST: %[[atan2:.*]] = math.atan2 %{{.*}}, %{{.*}}: f64
+! CHECK: %[[factor:.*]] = arith.constant 57.295779513082323 : f64
+! CHECK: %{{.*}} = arith.mulf %[[atan2]], %[[factor]] fastmath<contract> : f64

yi-wu-arm · 2024-01-22T16:14:43Z

Standard for atand:

16.9.25 ATAND (X) or ATAND (Y, X)
1 Description. Arc tangent function in degrees.
2 Class. Elemental function.
3 Arguments.
  Y shall be of type real.
  X If Y appears, X shall be of type real with the same kind type parameter as Y. If Y has the value
  zero, X shall not have the value zero. If Y does not appear, X shall be of type real.
4 Result Characteristics. Same as X.
5 Result Value. If Y appears, the result is the same as the result of ATAN2D (Y, X). If Y does not appear,
   the result has a value equal to a processor-dependent approximation to the arc tangent of X; it is expressed in
   degrees and lies in the range −90 ≤ ATAND (X) ≤ 90.
6 Example. ATAND (1.0) has the value 45.0 (approximately).

Standard for atan2d:

16.9.23 ATAN2D (Y, X)
1 Description. Arc tangent function in degrees.
2 Class. Elemental function.
3 Arguments.
  Y shall be of type real.
  X shall be of the same type and kind type parameter as Y. If Y has the value zero, X shall not have
  the value zero.
4 Result Characteristics. Same as X.
5 Result Value. The result is expressed in degrees and lies in the range −180 ≤ ATAN2D (Y, X) ≤ 180. It has
   a value equal to a processor-dependent approximation to ATAN2 (Y, X)×180/π.
6 Examples. ATAN2D (1.0, 1.0) has the value 45.0 (approximately)...

yi-wu-arm · 2024-01-22T16:16:37Z

I didn't add the result check on both atand(Y,X) and atan2d(Y,X), doesn't seem possible for them to have a value that doesn't lie between -180 <-> 180

flang/lib/Optimizer/Builder/IntrinsicCall.cpp

flang/lib/Evaluate/intrinsics.cpp

flang/lib/Optimizer/Builder/IntrinsicCall.cpp

flang/test/Lower/Intrinsics/atan2d.f90

kkwli · 2024-01-26T17:51:54Z

I should have pointed it out early. The same issue occurs in atand.f90 and atandpi.f90.

yi-wu-arm · 2024-01-26T18:49:37Z

I should have pointed it out early. The same issue occurs in atand.f90 and atandpi.f90.

Both atand and atanpi can have one or two input, I think its fine for the two-input version to have the name "_all_args`. Although usually "_all_input" was used for function/subroutine that has an optional argument.

flang/lib/Optimizer/Builder/IntrinsicCall.cpp

jeanPerier

Few new comments, LGTM otherwise.

flang/lib/Optimizer/Builder/IntrinsicCall.cpp

yi-wu-arm · 2024-01-31T17:32:41Z

build and pass all tests on Windows MSVC

…runtime as before

Co-authored-by: jeanPerier <jean.perier.polytechnique@gmail.com>

github-actions · 2024-02-05T10:14:42Z

⚠️ We detected that you are using a GitHub private e-mail address to contribute to the repo.
Please turn off Keep my email addresses private setting in your account.

yi-wu-arm · 2024-02-05T10:18:57Z

Hi, @jeanPerier sorry for throwing a ping, do you mind have a quick look at the updated code? I've changed DivOp to an InvPi with MulOp, and some small comment fix with rebase on main to solve merge conflict.

jeanPerier

LGTM, thanks for addressing my comments.

…pi(Y,X), atan2pi(Y,X) (llvm#79002) Fix: llvm#78568 --------- Co-authored-by: jeanPerier <jean.perier.polytechnique@gmail.com>

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir flang:semantics labels Jan 22, 2024

yi-wu-arm requested review from DavidTruby, jeanPerier and klausler January 22, 2024 16:11

yi-wu-arm commented Jan 22, 2024

View reviewed changes

flang/lib/Optimizer/Builder/IntrinsicCall.cpp Outdated Show resolved Hide resolved

klausler removed their request for review January 22, 2024 17:00

jeanPerier reviewed Jan 23, 2024

View reviewed changes

flang/lib/Evaluate/intrinsics.cpp Outdated Show resolved Hide resolved

flang/lib/Optimizer/Builder/IntrinsicCall.cpp Outdated Show resolved Hide resolved

yi-wu-arm changed the title ~~[flang] Fix for atand(Y,X), and implment atan2d(Y,X)~~ [flang] Fix for atand(Y,X), and implment atan2d(Y,X), atanpi(X), atanpi(Y,X), atan2pi(Y,X) Jan 26, 2024

kkwli reviewed Jan 26, 2024

View reviewed changes

flang/test/Lower/Intrinsics/atan2d.f90 Outdated Show resolved Hide resolved

flang/test/Lower/Intrinsics/atan2d.f90 Outdated Show resolved Hide resolved

yi-wu-arm requested a review from jeanPerier January 29, 2024 10:51

yi-wu-arm force-pushed the yi_dev_atand_fix branch 2 times, most recently from 9d91cf0 to 3805534 Compare January 29, 2024 15:40

yi-wu-arm requested a review from kkwli January 29, 2024 15:41

yi-wu-arm force-pushed the yi_dev_atand_fix branch from d201481 to 3a1f93d Compare January 30, 2024 10:40

jeanPerier reviewed Jan 30, 2024

View reviewed changes

flang/lib/Optimizer/Builder/IntrinsicCall.cpp Outdated Show resolved Hide resolved

jeanPerier reviewed Jan 31, 2024

View reviewed changes

yi-wu-arm requested a review from jeanPerier February 2, 2024 10:21

yi-wu-arm added 5 commits February 5, 2024 09:45

now accept two input with fake result: x/pi

2db83a2

add test and implement atan2d

239d013

add atan atanpi atan2pi and make the one input version accept --math-…

6e4d16e

…runtime as before

atanpi and atan2pi implementation and test fixes

dc81c5e

remove genatan for atan and atan2, no need to rewrite it

0fc11f3

yi-wu-arm and others added 9 commits February 5, 2024 10:05

format and comment

ae80d52

rename test functions

205d279

rename functions

caa1c49

more function renaming

3e418dc

remove zero termination check and correct test

a28bd0e

use inv_pi with mul instead of pi with div

4fc3a8c

Update flang/lib/Optimizer/Builder/IntrinsicCall.cpp

899be61

Co-authored-by: jeanPerier <jean.perier.polytechnique@gmail.com>

Update flang/lib/Optimizer/Builder/IntrinsicCall.cpp

24ce3c0

Co-authored-by: jeanPerier <jean.perier.polytechnique@gmail.com>

Update flang/lib/Optimizer/Builder/IntrinsicCall.cpp

5335907

Co-authored-by: jeanPerier <jean.perier.polytechnique@gmail.com>

yi-wu-arm force-pushed the yi_dev_atand_fix branch from c37a45d to 5335907 Compare February 5, 2024 10:13

jeanPerier approved these changes Feb 5, 2024

View reviewed changes

yi-wu-arm merged commit 1d3d893 into llvm:main Feb 5, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[flang] Fix for atand(Y,X), and implment atan2d(Y,X), atanpi(X), atanpi(Y,X), atan2pi(Y,X) #79002

[flang] Fix for atand(Y,X), and implment atan2d(Y,X), atanpi(X), atanpi(Y,X), atan2pi(Y,X) #79002

yi-wu-arm commented Jan 22, 2024 •

edited

llvmbot commented Jan 22, 2024 •

edited

yi-wu-arm commented Jan 22, 2024

yi-wu-arm commented Jan 22, 2024 •

edited

kkwli commented Jan 26, 2024

yi-wu-arm commented Jan 26, 2024 •

edited

jeanPerier left a comment

yi-wu-arm commented Jan 31, 2024

github-actions bot commented Feb 5, 2024

yi-wu-arm commented Feb 5, 2024

jeanPerier left a comment

[flang] Fix for atand(Y,X), and implment atan2d(Y,X), atanpi(X), atanpi(Y,X), atan2pi(Y,X) #79002

[flang] Fix for atand(Y,X), and implment atan2d(Y,X), atanpi(X), atanpi(Y,X), atan2pi(Y,X) #79002

Conversation

yi-wu-arm commented Jan 22, 2024 • edited

llvmbot commented Jan 22, 2024 • edited

yi-wu-arm commented Jan 22, 2024

yi-wu-arm commented Jan 22, 2024 • edited

kkwli commented Jan 26, 2024

yi-wu-arm commented Jan 26, 2024 • edited

jeanPerier left a comment

Choose a reason for hiding this comment

yi-wu-arm commented Jan 31, 2024

github-actions bot commented Feb 5, 2024

yi-wu-arm commented Feb 5, 2024

jeanPerier left a comment

Choose a reason for hiding this comment

yi-wu-arm commented Jan 22, 2024 •

edited

llvmbot commented Jan 22, 2024 •

edited

yi-wu-arm commented Jan 22, 2024 •

edited

yi-wu-arm commented Jan 26, 2024 •

edited