[Flang] Handle %VAL arguments correctly #157186

ceseo · 2025-09-05T21:46:55Z

Internal procedures expect reference parameters. However, when %VAL is used, the argument should be passed by value. Create a temporary variable in call generation and pass the address to avoid a type conversion error.

Fixes #118239

Internal procedures expect reference parameters. However, when %VAL is used, the argument should be passed by value. Create a temporary variable in call generation and pass the address to avoid a type conversion error. Fixes llvm#118239

llvmbot · 2025-09-05T21:48:25Z

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

Author: Carlos Seo (ceseo)

Changes

Internal procedures expect reference parameters. However, when %VAL is used, the argument should be passed by value. Create a temporary variable in call generation and pass the address to avoid a type conversion error.

Fixes #118239

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

2 Files Affected:

(modified) flang/lib/Lower/ConvertCall.cpp (+28-5)
(added) flang/test/Lower/percent-val-actual-argument.f90 (+14)

diff --git a/flang/lib/Lower/ConvertCall.cpp b/flang/lib/Lower/ConvertCall.cpp
index 04dcc9250be61..9f71f7417005f 100644
--- a/flang/lib/Lower/ConvertCall.cpp
+++ b/flang/lib/Lower/ConvertCall.cpp
@@ -494,10 +494,20 @@ Fortran::lower::genCallOpAndResult(
     // arguments of any type and vice versa.
     mlir::Value cast;
     auto *context = builder.getContext();
-    if (mlir::isa<fir::BoxProcType>(snd) &&
-        mlir::isa<mlir::FunctionType>(fst.getType())) {
-      auto funcTy = mlir::FunctionType::get(context, {}, {});
-      auto boxProcTy = builder.getBoxProcType(funcTy);
+
+    // Special handling for %VAL arguments: internal procedures expect
+    // reference parameters. When %VAL is used, the argument should be
+    // passed by value. So we need to create a temporary variable and
+    // pass its address to avoid a type conversion error.
+    if (fir::isa_ref_type(snd) && !fir::isa_ref_type(fst.getType()) &&
+        fir::dyn_cast_ptrEleTy(snd) == fst.getType()) {
+      mlir::Value temp = builder.createTemporary(loc, fst.getType());
+      builder.create<fir::StoreOp>(loc, fst, temp);
+      cast = temp;
+    } else if (mlir::isa<fir::BoxProcType>(snd) &&
+               mlir::isa<mlir::FunctionType>(fst.getType())) {
+      mlir::FunctionType funcTy = mlir::FunctionType::get(context, {}, {});
+      fir::BoxProcType boxProcTy = builder.getBoxProcType(funcTy);
       if (mlir::Value host = argumentHostAssocs(converter, fst)) {
         cast = fir::EmboxProcOp::create(builder, loc, boxProcTy,
                                         llvm::ArrayRef<mlir::Value>{fst, host});
@@ -1637,7 +1647,20 @@ void prepareUserCallArguments(
           (*cleanup)();
         break;
       }
-      caller.placeInput(arg, builder.createConvert(loc, argTy, value));
+      // For %VAL arguments, we should pass the value directly without
+      // conversion to reference types. If argTy is different from value type,
+      // it might be due to signature mismatch with internal procedures.
+      if (argTy == value.getType())
+        caller.placeInput(arg, value);
+      else if (fir::isa_ref_type(argTy) &&
+               fir::dyn_cast_ptrEleTy(argTy) == value.getType()) {
+        // We're trying to convert value to reference - create temporary
+        mlir::Value temp = builder.createTemporary(loc, value.getType());
+        builder.create<fir::StoreOp>(loc, value, temp);
+        caller.placeInput(arg, temp);
+      } else
+        caller.placeInput(arg, builder.createConvert(loc, argTy, value));
+
     } break;
     case PassBy::BaseAddressValueAttribute:
     case PassBy::CharBoxValueAttribute:
diff --git a/flang/test/Lower/percent-val-actual-argument.f90 b/flang/test/Lower/percent-val-actual-argument.f90
new file mode 100644
index 0000000000000..736baa0eca21d
--- /dev/null
+++ b/flang/test/Lower/percent-val-actual-argument.f90
@@ -0,0 +1,14 @@
+! RUN: bbc %s -emit-fir -o - | FileCheck %s
+
+program main
+  logical::a1
+  data a1/.true./
+  call sa(%val(a1))
+! CHECK: fir.load %3 : !fir.ref<!fir.logical<4>>
+! CHECK: fir.convert %13 : (!fir.logical<4>) -> i1
+  write(6,*) "a1 = ", a1
+end program main
+
+subroutine sa(x1)
+  logical::x1
+end subroutine sa

tblah

Thanks for the bug fix

flang/test/Lower/percent-val-actual-argument.f90

flang/lib/Lower/ConvertCall.cpp

flang/test/Lower/percent-val-actual-argument.f90

* Fix test to reflect the passing by loaded value change. * Check the actual call. * Use flang -fc1 -emit-hlfir instead of bbc.

tblah · 2025-09-08T16:19:44Z

flang/lib/Lower/ConvertCall.cpp

-      mlir::Value temp = builder.createTemporary(loc, fst.getType());
-      builder.create<fir::StoreOp>(loc, fst, temp);
-      cast = temp;
+      auto loadOp = mlir::cast<fir::LoadOp>(fst.getDefiningOp());


Thanks for the update. Please could you keep the old code creating the temporary as a fallback. I'm not 100% certain that there will always be a load here. If the cast failed that would be a compiler crash. I'd rather have 2 lines of dead code than a compiler crash for some obscure untested case.

I think that's the case because hlfir::loadTrivialScalar always creates fir::LoadOp for scalar trivial variables coming from hlfir.declare.

Checking I understand the reasoning here: do you mean it will have always taken this path, and that means it must be a load op?

llvm-project/flang/lib/Lower/ConvertCall.cpp

Line 1613 in 747ede1

hlfir::Entity value = hlfir::loadTrivialScalar(loc, builder, actual);

Yes, correct.

tblah

Thank you for the bug fix

ceseo · 2025-09-09T18:46:40Z

@tblah I noticed this is breaking the gfortran testsuite: https://lab.llvm.org/buildbot/#/builders/4/builds/9037

The test is:

! { dg-do run }                                                                 
! { dg-options "-fcheck=all" }                                                  
!                                                                               
! { dg-shouldfail "Pointer check" }                                             
! { dg-output "Fortran runtime error: Pointer actual argument 'p' is not associated" }
!                                                                               
! PR fortran/50718                                                              
!                                                                               
! Was failing with -fcheck=pointer: Segfault at run time                        
                                                                                
integer, pointer :: p => null()                                                 
                                                                                
call sub2(%val(p)) ! Invalid: Nonassociated pointer                             
end                                                                             
                                                                                
! Not quite correct dummy, but if one uses VALUE, gfortran                      
! complains about a missing interface - which we cannot use                     
! if we want to use %VAL().                                                     
                                                                                
subroutine sub2(p)                                                              
  integer :: p                                                                  
end subroutine sub2

I don't think this applies to Flang. What do you think?

tblah · 2025-09-10T10:54:06Z

Agreed. This seems to be testing for a compile error which we don't implement. Previously this passed because flang crashed (so it looked to the test suite like the compile didn't succeed). Now that you've fixed %VAL this will compile correctly (and segfault at runtime). I think this test should be disabled.

jeanPerier · 2025-09-10T13:34:59Z

Hi @ceseo, thanks for working on the %VAL fix. I agree a segfault is the expectation for the gfortran test, however, I do not think people will get it with the fix you did [edit: because it removed the load from the pointer target data].

I think removing the load is not the correct approach because this will cause the compilation of the caller to be lowered differently if the implicit procedure is or not defined in the same file, while implicit calls should not be impacted by the existence of the definition of the definition in the same file. This is because the "compilation unit" concept is not really a Fortran concept, it is an implementation detail. Code move from one compilation unit to another should not affect the semantic of a program.

Basically, we should lower test1 and test2 the same way in this example (just like gfortran) and I do not think it is the case with your fix (see godbolt link).

subroutine sa(x1)
  logical::x1
end

subroutine test1(a1)
  logical::a1
  ! Implicit call. sa1 defined in the current compilation unit.
  call sa(%val(a1))
end

subroutine test2(a2)
  logical::a2
  ! Implicit call. sa2 not defined in compilation unit.
  call sa2(%val(a2))
end

I think the correct fix is to force an indirect call to the implicit signature just like mustCastFuncOpToCopeWithImplicitInterfaceMismatch is doing for cases without %VAL. There may be a %VAL issue around that logic.

ceseo · 2025-09-10T14:04:44Z

I think the correct fix is to force an indirect call to the implicit signature just like mustCastFuncOpToCopeWithImplicitInterfaceMismatch is doing for cases without %VAL. There may be a %VAL issue around that logic.

Ah, I see. I'll take a look at this. Thanks!

ceseo · 2025-09-10T15:00:33Z

New PR submitted: #157873

Thanks for your input!

eugeneepshteyn · 2025-09-10T15:01:19Z

@ceseo , this is probably related to Jean's comment above, this Fujitsu test now fails: https://github.com/fujitsu/compiler-test-suite/blob/main/Fortran/0977/0977_0061.f90

It prints "NG" for calls to s() and ssss(). When succeeding, it should only print "pass".

sscalpone · 2025-09-10T15:18:25Z

%VAL is an extension. Intel and gfortran have different behavior. What behavior do we want in flang?

ceseo · 2025-09-10T15:27:19Z

@ceseo , this is probably related to Jean's comment above, this Fujitsu test now fails: https://github.com/fujitsu/compiler-test-suite/blob/main/Fortran/0977/0977_0061.f90

It prints "NG" for calls to s() and ssss(). When succeeding, it should only print "pass".

Yes, it is. I just confirmed the PR I submitted above fixes it too.

klausler · 2025-09-10T15:34:01Z

what is the difference between %VAL(x) and (x) as an actual argument?

eugeneepshteyn · 2025-09-10T15:50:39Z

what is the difference between %VAL(x) and (x) as an actual argument?

Wouldn't (x) require a temporary? %VAL(x) should just load the value and pass it as is.

eugeneepshteyn · 2025-09-10T15:51:53Z

%VAL is an extension. Intel and gfortran have different behavior. What behavior do we want in flang?

I believe Intel places some restrictions on what could be done with %VAL: https://www.intel.com/content/www/us/en/docs/fortran-compiler/developer-guide-reference/2025-2/val.html

How does gfortran behave?

[Flang] Handle %VAL arguments correctly

d7f5c64

Internal procedures expect reference parameters. However, when %VAL is used, the argument should be passed by value. Create a temporary variable in call generation and pass the address to avoid a type conversion error. Fixes llvm#118239

llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Sep 5, 2025

ceseo requested a review from tblah September 5, 2025 22:12

tblah reviewed Sep 8, 2025

View reviewed changes

ceseo added 3 commits September 8, 2025 15:10

Remove unnecessary temporary storage creation

747ede1

Fix the testcase to reflect latest changes

b65fad2

* Fix test to reflect the passing by loaded value change. * Check the actual call. * Use flang -fc1 -emit-hlfir instead of bbc.

Add a new test to check passing %VAL to a VALUE argument

cf32305

tblah reviewed Sep 8, 2025

View reviewed changes

tblah approved these changes Sep 9, 2025

View reviewed changes

ceseo merged commit 225f181 into llvm:main Sep 9, 2025
9 checks passed

[Flang] Handle %VAL arguments correctly #157186

[Flang] Handle %VAL arguments correctly #157186

Uh oh!

Conversation

ceseo commented Sep 5, 2025

Uh oh!

llvmbot commented Sep 5, 2025

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tblah Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

ceseo Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

tblah Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

ceseo Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

tblah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ceseo commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tblah commented Sep 10, 2025

Uh oh!

jeanPerier commented Sep 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ceseo commented Sep 10, 2025

Uh oh!

ceseo commented Sep 10, 2025

Uh oh!

eugeneepshteyn commented Sep 10, 2025

Uh oh!

sscalpone commented Sep 10, 2025

Uh oh!

ceseo commented Sep 10, 2025

Uh oh!

klausler commented Sep 10, 2025

Uh oh!

eugeneepshteyn commented Sep 10, 2025

Uh oh!

eugeneepshteyn commented Sep 10, 2025

Uh oh!

Uh oh!

ceseo commented Sep 9, 2025 •

edited

Loading

jeanPerier commented Sep 10, 2025 •

edited

Loading