From eb68c7b68edccbb44b997ba01f535e39a9801b09 Mon Sep 17 00:00:00 2001
From: jeanPerier <jperier@nvidia.com>
Date: Thu, 18 Jul 2024 09:36:13 +0200
Subject: [PATCH] [flang] load SECOND result in genSecond (#99342)

Until genSecond, all intrinsic `genXXX` returning scalar intrinsic
(except NULL) were returning them as value.

The code calling genIntrinsicCall is using that assumption when
generation the asExprOp because hflir.expr<> of scalar are badly
supported in tools (I should likely just forbid them all together), the
type is meant for "non trivial" values: arrays, character, and derived
type. For instance, the added tests crashed with error: `'arith.subf' op
operand #0 must be floating-point-like, but got '!hlfir.expr<f32>'`

Load the result in genSecond and add an assert after genIntrinsicCall to
better enforce this.
---
 flang/lib/Lower/ConvertCall.cpp               |  2 ++
 flang/lib/Optimizer/Builder/IntrinsicCall.cpp |  2 +-
 flang/test/Lower/Intrinsics/second.f90        | 28 +++++++++++++++----
 3 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/flang/lib/Lower/ConvertCall.cpp b/flang/lib/Lower/ConvertCall.cpp
index 54e29a1d60689e4..ba65b644e5a93bf 100644
--- a/flang/lib/Lower/ConvertCall.cpp
+++ b/flang/lib/Lower/ConvertCall.cpp
@@ -2005,6 +2005,8 @@ genIntrinsicRefCore(Fortran::lower::PreparedActualArguments &loweredActuals,
   // returns a null pointer variable that should not be transformed into a value
   // (what matters is the memory address).
   if (resultEntity.isVariable() && intrinsicName != "null") {
+    assert(!fir::isa_trivial(fir::unwrapRefType(resultEntity.getType())) &&
+           "expect intrinsic scalar results to not be in memory");
     hlfir::AsExprOp asExpr;
     // Character/Derived MERGE lowering returns one of its argument address
     // (this is the only intrinsic implemented in that way so far). The
diff --git a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
index ba71fb3b4040c59..0e5e30a7024d871 100644
--- a/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
+++ b/flang/lib/Optimizer/Builder/IntrinsicCall.cpp
@@ -6161,7 +6161,7 @@ IntrinsicLibrary::genSecond(std::optional<mlir::Type> resultType,
   genCpuTime(subroutineArgs);
 
   if (resultType)
-    return result;
+    return builder.create<fir::LoadOp>(loc, fir::getBase(result));
   return {};
 }
 
diff --git a/flang/test/Lower/Intrinsics/second.f90 b/flang/test/Lower/Intrinsics/second.f90
index f1e66506aaaca97..7c5cc5e09bbe6d4 100644
--- a/flang/test/Lower/Intrinsics/second.f90
+++ b/flang/test/Lower/Intrinsics/second.f90
@@ -28,10 +28,28 @@ subroutine test_function(time)
 ! CHECK:           %[[VAL_4:.*]] = fir.call @_FortranACpuTime() fastmath<contract> : () -> f64
 ! CHECK:           %[[VAL_5:.*]] = fir.convert %[[VAL_4]] : (f64) -> f32
 ! CHECK:           fir.store %[[VAL_5]] to %[[VAL_1]] : !fir.ref<f32>
-! CHECK:           %[[VAL_6:.*]]:2 = hlfir.declare %[[VAL_1]] {uniq_name = ".tmp.intrinsic_result"} : (!fir.ref<f32>) -> (!fir.ref<f32>, !fir.ref<f32>)
-! CHECK:           %[[VAL_7:.*]] = arith.constant false
-! CHECK:           %[[VAL_8:.*]] = hlfir.as_expr %[[VAL_6]]#0 move %[[VAL_7]] : (!fir.ref<f32>, i1) -> !hlfir.expr<f32>
-! CHECK:           hlfir.assign %[[VAL_8]] to %[[VAL_3]]#0 : !hlfir.expr<f32>, !fir.ref<f32>
-! CHECK:           hlfir.destroy %[[VAL_8]] : !hlfir.expr<f32>
+! CHECK:           %[[VAL_6:.*]] = fir.load %[[VAL_1]] : !fir.ref<f32>
+! CHECK:           hlfir.assign %[[VAL_6]] to %[[VAL_3]]#0 : f32, !fir.ref<f32>
+! CHECK:           return
+! CHECK:         }
+
+subroutine test_function_subexpr(t1, t2)
+  real :: t1, t2
+  t2 = second() - t1
+end subroutine
+! CHECK-LABEL:   func.func @_QPtest_function_subexpr(
+! CHECK-SAME:                                        %[[VAL_0:.*]]: !fir.ref<f32> {fir.bindc_name = "t1"},
+! CHECK-SAME:                                        %[[VAL_1:.*]]: !fir.ref<f32> {fir.bindc_name = "t2"}) {
+! CHECK:           %[[VAL_2:.*]] = fir.alloca f32
+! CHECK:           %[[VAL_3:.*]] = fir.dummy_scope : !fir.dscope
+! CHECK:           %[[VAL_4:.*]]:2 = hlfir.declare %[[VAL_0]] dummy_scope %[[VAL_3]] {uniq_name = "_QFtest_function_subexprEt1"} : (!fir.ref<f32>, !fir.dscope) -> (!fir.ref<f32>, !fir.ref<f32>)
+! CHECK:           %[[VAL_5:.*]]:2 = hlfir.declare %[[VAL_1]] dummy_scope %[[VAL_3]] {uniq_name = "_QFtest_function_subexprEt2"} : (!fir.ref<f32>, !fir.dscope) -> (!fir.ref<f32>, !fir.ref<f32>)
+! CHECK:           %[[VAL_6:.*]] = fir.call @_FortranACpuTime() fastmath<contract> : () -> f64
+! CHECK:           %[[VAL_7:.*]] = fir.convert %[[VAL_6]] : (f64) -> f32
+! CHECK:           fir.store %[[VAL_7]] to %[[VAL_2]] : !fir.ref<f32>
+! CHECK:           %[[VAL_8:.*]] = fir.load %[[VAL_2]] : !fir.ref<f32>
+! CHECK:           %[[VAL_9:.*]] = fir.load %[[VAL_4]]#0 : !fir.ref<f32>
+! CHECK:           %[[VAL_10:.*]] = arith.subf %[[VAL_8]], %[[VAL_9]] fastmath<contract> : f32
+! CHECK:           hlfir.assign %[[VAL_10]] to %[[VAL_5]]#0 : f32, !fir.ref<f32>
 ! CHECK:           return
 ! CHECK:         }