[flang] load SECOND result in genSecond

[jeanPerier]

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.

[llvmbot]

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

Author: None (jeanPerier)

Changes

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&lt;f32&gt;'

Load the result in genSecond and add an assert after genIntrinsicCall to better enforce this.

---

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

3 Files Affected:

• (modified) flang/lib/Lower/ConvertCall.cpp (+2)
• (modified) flang/lib/Optimizer/Builder/IntrinsicCall.cpp (+1-1)
• (modified) flang/test/Lower/Intrinsics/second.f90 (+23-5)

diff --git a/flang/lib/Lower/ConvertCall.cpp b/flang/lib/Lower/ConvertCall.cpp
index 54e29a1d60689..ba65b644e5a93 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 e12e21bb00e15..be25b8d623c45 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 f1e66506aaaca..7c5cc5e09bbe6 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:         }

[psteinfeld]

All builds and tests correctly and looks good.

[tblah]

Thanks for catching this. LGTM