Skip to content

[flang][hlfir] patch for assumed shape dummy with VALUE keyword when lowering to HLFIR #70391

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 8, 2023

Conversation

cabreraam
Copy link
Contributor

@cabreraam cabreraam commented Oct 26, 2023

Adds functionality for assumed shape dummy with value keyword when lowering to HLFIR

@cabreraam cabreraam requested a review from jeanPerier October 26, 2023 23:26
@llvmbot llvmbot added flang Flang issues not falling into any other category flang:fir-hlfir labels Oct 26, 2023
@llvmbot
Copy link
Member

llvmbot commented Oct 26, 2023

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

Author: Anthony Cabrera (cabreraam)

Changes

Adds functionality for assumed shape dummy with value keyword


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

2 Files Affected:

  • (modified) flang/lib/Lower/CallInterface.cpp (+8-4)
  • (added) flang/test/HLFIR/assumed_shape_with_value_keyword.f90 (+149)
diff --git a/flang/lib/Lower/CallInterface.cpp b/flang/lib/Lower/CallInterface.cpp
index 379d9be0e53a3dc..2ebc2f5b6612103 100644
--- a/flang/lib/Lower/CallInterface.cpp
+++ b/flang/lib/Lower/CallInterface.cpp
@@ -964,10 +964,14 @@ class Fortran::lower::CallInterfaceImpl {
       addPassedArg(PassEntityBy::MutableBox, entity, characteristics);
     } else if (dummyRequiresBox(obj, isBindC)) {
       // Pass as fir.box or fir.class
-      if (isValueAttr)
-        TODO(loc, "assumed shape dummy argument with VALUE attribute");
-      addFirOperand(boxType, nextPassedArgPosition(), Property::Box, attrs);
-      addPassedArg(PassEntityBy::Box, entity, characteristics);
+      Property prop = Property::Box;
+      PassEntityBy passBy = PassEntityBy::Box;
+      if (isValueAttr) {
+        passBy = PassEntityBy::BaseAddressValueAttribute;
+        prop = Property::Value;
+      }
+      addFirOperand(boxType, nextPassedArgPosition(), prop, attrs);
+      addPassedArg(passBy, entity, characteristics);
     } else if (dynamicType.category() ==
                Fortran::common::TypeCategory::Character) {
       // Pass as fir.box_char
diff --git a/flang/test/HLFIR/assumed_shape_with_value_keyword.f90 b/flang/test/HLFIR/assumed_shape_with_value_keyword.f90
new file mode 100644
index 000000000000000..b5080d9bedca690
--- /dev/null
+++ b/flang/test/HLFIR/assumed_shape_with_value_keyword.f90
@@ -0,0 +1,149 @@
+! RUN: bbc -emit-hlfir %s -o - | FileCheck %s
+
+! Addresses assumed shape dummy argument with VALUE keyword
+
+subroutine test_integer_value1(x)
+  integer, value :: x(:)
+  call internal_call1(x)
+end
+
+! CHECK-LABEL:  func.func @_QPtest_integer_value1(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?xi32>> {fir.bindc_name = "x"}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<value>, uniq_name = "_QFtest_integer_value1Ex"} : (!fir.box<!fir.array<?xi32>>) -> (!fir.box<!fir.array<?xi32>>, !fir.box<!fir.array<?xi32>>)
+! CHECK:          %[[VAL_1:.*]]:2 = hlfir.copy_in %[[VAL_0]]#0 : (!fir.box<!fir.array<?xi32>>) -> (!fir.box<!fir.array<?xi32>>, i1)
+! CHECK:          %[[VAL_2:.*]] = fir.box_addr %[[VAL_1]]#0 : (!fir.box<!fir.array<?xi32>>) -> !fir.ref<!fir.array<?xi32>>
+! CHECK:          fir.call @_QPinternal_call1(%[[VAL_2]]) fastmath<contract> : (!fir.ref<!fir.array<?xi32>>) -> ()
+! CHECK:          hlfir.copy_out %[[VAL_1]]#0, %[[VAL_1]]#1 to %[[VAL_0]]#0 : (!fir.box<!fir.array<?xi32>>, i1, !fir.box<!fir.array<?xi32>>) -> ()
+! CHECK:          return
+! CHECK:        }
+
+subroutine test_integer_value2(x)
+  integer, value :: x(:,:)
+  call internal_call2(x)
+end
+! CHECK-LABEL:  func.func @_QPtest_integer_value2(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?x?xi32>> {fir.bindc_name = "x"}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<value>, uniq_name = "_QFtest_integer_value2Ex"} : (!fir.box<!fir.array<?x?xi32>>) -> (!fir.box<!fir.array<?x?xi32>>, !fir.box<!fir.array<?x?xi32>>)
+! CHECK:          %[[VAL_1:.*]]:2 = hlfir.copy_in %[[VAL_0]]#0 : (!fir.box<!fir.array<?x?xi32>>) -> (!fir.box<!fir.array<?x?xi32>>, i1)
+! CHECK:          %[[VAL_2:.*]] = fir.box_addr %[[VAL_1]]#0 : (!fir.box<!fir.array<?x?xi32>>) -> !fir.ref<!fir.array<?x?xi32>>
+! CHECK:          fir.call @_QPinternal_call2(%[[VAL_2]]) fastmath<contract> : (!fir.ref<!fir.array<?x?xi32>>) -> ()
+! CHECK:          hlfir.copy_out %[[VAL_1]]#0, %[[VAL_1]]#1 to %[[VAL_0]]#0 : (!fir.box<!fir.array<?x?xi32>>, i1, !fir.box<!fir.array<?x?xi32>>) -> ()
+! CHECK:          return
+! CHECK:        }
+
+subroutine test_real_value1(x) 
+  real, value :: x(:)
+  call internal_call3(x)
+end
+! CHECK-LABEL:  func.func @_QPtest_real_value1(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "x"}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<value>, uniq_name = "_QFtest_real_value1Ex"} : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, !fir.box<!fir.array<?xf32>>)
+! CHECK:          %[[VAL_1:.*]]:2 = hlfir.copy_in %[[VAL_0]]#0 : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, i1)
+! CHECK:          %[[VAL_2:.*]] = fir.box_addr %[[VAL_1]]#0 : (!fir.box<!fir.array<?xf32>>) -> !fir.ref<!fir.array<?xf32>>
+! CHECK:          fir.call @_QPinternal_call3(%[[VAL_2]]) fastmath<contract> : (!fir.ref<!fir.array<?xf32>>) -> ()
+! CHECK:          hlfir.copy_out %[[VAL_1]]#0, %[[VAL_1]]#1 to %[[VAL_0]]#0 : (!fir.box<!fir.array<?xf32>>, i1, !fir.box<!fir.array<?xf32>>) -> ()
+! CHECK:          return
+! CHECK:        }
+
+subroutine test_real_value2(x) 
+  real, value :: x(:,:)
+  call internal_call4(x)
+end
+! CHECK-LABEL:  func.func @_QPtest_real_value2(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?x?xf32>> {fir.bindc_name = "x"}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<value>, uniq_name = "_QFtest_real_value2Ex"} : (!fir.box<!fir.array<?x?xf32>>) -> (!fir.box<!fir.array<?x?xf32>>, !fir.box<!fir.array<?x?xf32>>)
+! CHECK:          %[[VAL_1:.*]]:2 = hlfir.copy_in %[[VAL_0]]#0 : (!fir.box<!fir.array<?x?xf32>>) -> (!fir.box<!fir.array<?x?xf32>>, i1)
+! CHECK:          %[[VAL_2:.*]] = fir.box_addr %[[VAL_1]]#0 : (!fir.box<!fir.array<?x?xf32>>) -> !fir.ref<!fir.array<?x?xf32>>
+! CHECK:          fir.call @_QPinternal_call4(%[[VAL_2]]) fastmath<contract> : (!fir.ref<!fir.array<?x?xf32>>) -> ()
+! CHECK:          hlfir.copy_out %[[VAL_1]]#0, %[[VAL_1]]#1 to %[[VAL_0]]#0 : (!fir.box<!fir.array<?x?xf32>>, i1, !fir.box<!fir.array<?x?xf32>>) -> ()
+! CHECK:          return
+! CHECK:        }
+
+subroutine test_complex_value1(x)
+  complex, value :: x(:)
+  call internal_call5(x)
+end
+! CHECK-LABEL:  func.func @_QPtest_complex_value1(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?x!fir.complex<4>>> {fir.bindc_name = "x"}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<value>, uniq_name = "_QFtest_complex_value1Ex"} : (!fir.box<!fir.array<?x!fir.complex<4>>>) -> (!fir.box<!fir.array<?x!fir.complex<4>>>, !fir.box<!fir.array<?x!fir.complex<4>>>)
+! CHECK:          %[[VAL_1:.*]]:2 = hlfir.copy_in %[[VAL_0]]#0 : (!fir.box<!fir.array<?x!fir.complex<4>>>) -> (!fir.box<!fir.array<?x!fir.complex<4>>>, i1)
+! CHECK:          %[[VAL_2:.*]] = fir.box_addr %[[VAL_1]]#0 : (!fir.box<!fir.array<?x!fir.complex<4>>>) -> !fir.ref<!fir.array<?x!fir.complex<4>>>
+! CHECK:          fir.call @_QPinternal_call5(%[[VAL_2]]) fastmath<contract> : (!fir.ref<!fir.array<?x!fir.complex<4>>>) -> ()
+! CHECK:          hlfir.copy_out %[[VAL_1]]#0, %[[VAL_1]]#1 to %[[VAL_0]]#0 : (!fir.box<!fir.array<?x!fir.complex<4>>>, i1, !fir.box<!fir.array<?x!fir.complex<4>>>) -> ()
+! CHECK:          return
+! CHECK:        }
+
+subroutine test_complex_value2(x)
+  complex, value :: x(:,:)
+  call internal_call6(x)
+end
+! CHECK-LABEL:  func.func @_QPtest_complex_value2(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?x?x!fir.complex<4>>> {fir.bindc_name = "x"}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<value>, uniq_name = "_QFtest_complex_value2Ex"} : (!fir.box<!fir.array<?x?x!fir.complex<4>>>) -> (!fir.box<!fir.array<?x?x!fir.complex<4>>>, !fir.box<!fir.array<?x?x!fir.complex<4>>>)
+! CHECK:          %[[VAL_1:.*]]:2 = hlfir.copy_in %[[VAL_0]]#0 : (!fir.box<!fir.array<?x?x!fir.complex<4>>>) -> (!fir.box<!fir.array<?x?x!fir.complex<4>>>, i1)
+! CHECK:          %[[VAL_2:.*]] = fir.box_addr %[[VAL_1]]#0 : (!fir.box<!fir.array<?x?x!fir.complex<4>>>) -> !fir.ref<!fir.array<?x?x!fir.complex<4>>>
+! CHECK:          fir.call @_QPinternal_call6(%[[VAL_2]]) fastmath<contract> : (!fir.ref<!fir.array<?x?x!fir.complex<4>>>) -> ()
+! CHECK:          hlfir.copy_out %[[VAL_1]]#0, %[[VAL_1]]#1 to %[[VAL_0]]#0 : (!fir.box<!fir.array<?x?x!fir.complex<4>>>, i1, !fir.box<!fir.array<?x?x!fir.complex<4>>>) -> ()
+! CHECK:          return
+! CHECK:        }
+
+subroutine test_optional1(x)
+  real, value, optional :: x(:)
+  if (present(x)) then
+    call internal_call7(x)
+  endif
+end
+! CHECK-LABEL:  func.func @_QPtest_optional1(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "x", fir.optional}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<optional, value>, uniq_name = "_QFtest_optional1Ex"} : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, !fir.box<!fir.array<?xf32>>)
+! CHECK:          %[[VAL_1:.*]] = fir.is_present %[[VAL_0]]#1 : (!fir.box<!fir.array<?xf32>>) -> i1
+! CHECK:          fir.if %[[VAL_1:.*]] {
+! CHECK:            %[[VAL_2:.*]]:2 = hlfir.copy_in %[[VAL_0]]#0 : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, i1)
+! CHECK:            %[[VAL_3:.*]] = fir.box_addr %[[VAL_2]]#0 : (!fir.box<!fir.array<?xf32>>) -> !fir.ref<!fir.array<?xf32>>
+! CHECK:            fir.call @_QPinternal_call7(%[[VAL_3]]) fastmath<contract> : (!fir.ref<!fir.array<?xf32>>) -> ()
+! CHECK:            hlfir.copy_out %[[VAL_2]]#0, %[[VAL_2]]#1 to %[[VAL_0]]#0 : (!fir.box<!fir.array<?xf32>>, i1, !fir.box<!fir.array<?xf32>>) -> ()
+! CHECK:          } else {
+! CHECK:          }
+! CHECK:          return
+! CHECK:        }
+
+subroutine test_optional2(x)
+  real, value, optional :: x(:,:)
+  if (present(x)) then
+    call internal_call8(x)
+  endif
+end
+! CHECK-LABEL:  func.func @_QPtest_optional2(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?x?xf32>> {fir.bindc_name = "x", fir.optional}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<optional, value>, uniq_name = "_QFtest_optional2Ex"} : (!fir.box<!fir.array<?x?xf32>>) -> (!fir.box<!fir.array<?x?xf32>>, !fir.box<!fir.array<?x?xf32>>)
+! CHECK:          %[[VAL_1:.*]] = fir.is_present %[[VAL_0]]#1 : (!fir.box<!fir.array<?x?xf32>>) -> i1
+! CHECK:          fir.if %[[VAL_1:.*]] {
+! CHECK:            %[[VAL_2:.*]]:2 = hlfir.copy_in %[[VAL_0]]#0 : (!fir.box<!fir.array<?x?xf32>>) -> (!fir.box<!fir.array<?x?xf32>>, i1)
+! CHECK:            %[[VAL_3:.*]] = fir.box_addr %[[VAL_2]]#0 : (!fir.box<!fir.array<?x?xf32>>) -> !fir.ref<!fir.array<?x?xf32>>
+! CHECK:            fir.call @_QPinternal_call8(%[[VAL_3]]) fastmath<contract> : (!fir.ref<!fir.array<?x?xf32>>) -> ()
+! CHECK:            hlfir.copy_out %[[VAL_2]]#0, %[[VAL_2]]#1 to %[[VAL_0]]#0 : (!fir.box<!fir.array<?x?xf32>>, i1, !fir.box<!fir.array<?x?xf32>>) -> ()
+! CHECK:          } else {
+! CHECK:          }
+! CHECK:          return
+! CHECK:        }
+
+subroutine test_optional3(x)
+  real, value, optional :: x(:)
+  if (present(x)) then
+    stop
+  endif
+end
+! CHECK-LABEL:  func.func @_QPtest_optional3(
+! CHECK-SAME:     %[[ARG0:.*]]: !fir.box<!fir.array<?xf32>> {fir.bindc_name = "x", fir.optional}) {
+! CHECK:          %[[VAL_0:.*]]:2 = hlfir.declare %[[ARG0]] {fortran_attrs = #fir.var_attrs<optional, value>, uniq_name = "_QFtest_optional3Ex"} : (!fir.box<!fir.array<?xf32>>) -> (!fir.box<!fir.array<?xf32>>, !fir.box<!fir.array<?xf32>>)
+! CHECK:          %[[VAL_1:.*]] = fir.is_present %[[VAL_0]]#1 : (!fir.box<!fir.array<?xf32>>) -> i1
+! CHECK:          cf.cond_br %[[VAL_1]], ^bb1, ^bb2
+! CHECK:          b1:  // pred: ^bb0
+! CHECK:          %[[C0_I32:.*]] = arith.constant 0 : i32
+! CHECK:          %[[FALSE:.*]] = arith.constant false
+! CHECK:          %[[FALSE_0:.*]] = arith.constant false
+! CHECK:          %[[VAL_2:.*]] = fir.call @_FortranAStopStatement(%[[C0_I32]], %[[FALSE]], %[[FALSE]]_0) fastmath<contract> : (i32, i1, i1) -> none
+! CHECK:          fir.unreachable
+! CHECK:          b2:  // pred: ^bb0
+! CHECK:          return
+! CHECK:        }
\ No newline at end of file

@cabreraam cabreraam force-pushed the assumed_shape_w_value branch from 3f7dbea to fe845c7 Compare October 27, 2023 01:38
Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this.

Comment on lines 980 to 985
Property prop = Property::Box;
PassEntityBy passBy = PassEntityBy::Box;
if (isValueAttr) {
passBy = PassEntityBy::BaseAddressValueAttribute;
prop = Property::Value;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PassEntityBy::BaseAddressValueAttribute and Property::Value are incorrect here (they first implies the argument should be prepared as a raw address for a VALUE, and the second says that the FIR argument at that position is a value in register).

This makes no difference on the callee side, but if you add tests that calls procedure with such arguments, there will likely be issues since BaseAddressValueAttribute implies the base address should be passed.

I think you will want to just keep it as it is, the VALUE attribute is checked in HLFIR here

if (arg.hasValueAttribute() ||
.

Without HLFIR, there is a need for a new BoxValueAttribute category and special handling for it (that is why I think the TODO should only be lifted when lowering to HLFIR converter.getLoweringOptions().getLowerToHighLevelFIR()).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I integrated this feedback into a new commit. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update, please remove the commented code (and I would avoid creating the "prop" and "passBy" variable and keep the original code instead). Otherwise LGTM.

@cabreraam cabreraam force-pushed the assumed_shape_w_value branch from fe845c7 to 67fcba2 Compare October 31, 2023 18:41
@cabreraam cabreraam changed the title [flang][hlfir] patch for assumed shape dummy with VALUE keyword [flang][hlfir] patch for assumed shape dummy with VALUE keyword when lowering to HLFIR Oct 31, 2023
@cabreraam cabreraam force-pushed the assumed_shape_w_value branch from 67fcba2 to 74ba369 Compare November 7, 2023 18:10
@cabreraam cabreraam requested a review from jeanPerier November 7, 2023 21:59
Copy link
Contributor

@jeanPerier jeanPerier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! LGTM

@cabreraam cabreraam merged commit 8803211 into llvm:main Nov 8, 2023
@cabreraam cabreraam deleted the assumed_shape_w_value branch November 8, 2023 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flang:fir-hlfir flang Flang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants