[flang][hlfir] Create temporary for passing constant expression for actual arg.

[vzakhari]

Summary

[flang][hlfir] Create temporary for passing constant expression for actual arg.

Even though the constant expression actual argument is not definable,
and the associated dummy argument is not definable, the compiler may produce
implicit copies into the memory storage associated with the constant expression.
For example, a constant expression storage passed by reference to a subprogram
may be used for implicit copy-out:

subroutine sub(i, n)
  interface
     subroutine sub2(i)
       integer :: i(*)
     end subroutine sub2
  end interface
  integer :: i(n)
  call sub2(i(3::2)) ! copy-out after the call will write to 'i'
end subroutine sub
subroutine test
  call sub((/1,2,3,4,5/), 5)
end subroutine test

If we pass a reference to constant expression storage to 'sub' directly,
the copy-out inside 'sub' will try to write into readonly memory.

[everythingfunctional]

A program that did this would be invalid. Wouldn't you want it to crash, rather than silently do something else?

[vzakhari]

In D151271#4366595, @everythingfunctional wrote:

A program that did this would be invalid. Wouldn't you want it to crash, rather than silently do something else?

Hi @everythingfunctional, the program in the commit message seems conformant to me. I did not find anything in the standard that would say otherwise. Please note that the there is no definition/redefinition of i inside sub2. I will appreciate it if you can list a set of statements from the standard that make this program invalid.

[everythingfunctional]

It depends on what sub2 tries to do with i. Putting the relevant pieces together:

3.38
constant
data object that has a value and which cannot be defined, redefined, or become undefined during execution of a program (6.2.3, 9.3)

3.63
dummy argument
entity whose identifier appears in a dummy argument list ...

6.2.3 Constants
R604 *constant* is literal-constant or named-constant

9.3 Constants
... redefinition of a constant is never permitted

8.5.10 INTENT attribute
The INTENT attribute specifies the intended use of a dummy argument.
...
The INTENT (IN) attribute for a nonpointer dummy argument specifies that it shall neither be defined nor become undefined during the invocation and execution of the procedure.

So you can have constant as the actual argument for an INTENT(IN) dummy argument.

The INTENT (OUT) attribute for a nonpointer dummy argument specifies that the dummy argument becomes undefined on invocation of the procedure

So you can't have a constant as the actual argument for an INTENT(OUT) dummy argument.

The INTENT (INOUT) attribute for a nonpointer dummy argument specifies that any actual argument that corresponds to the dummy argument shall be definable.

So you can't have a constant as the actual argument for an INTENT(INOUT) dummy argument either

If no INTENT attribute is specified for a dummy argument, its use is subject to the limitations of its effective argument (15.5.2).

Meaning so long as the procedure doesn't try to execute a statement that would cause the dummy argument to be undefined or redefined you can have a constant as the actual argument. But you seem to indicate in your discussion that you believe sub2 intends to redefine i. If it does that would be invalid.

I will note that the key statement there is in normative text, so a processor is not required to detect it (and it would be impossible to detect at compile time in all cases), but you shouldn't go out of your way to avoid detecting it at run time.

[everythingfunctional]

In D151271#4368507, @vzakhari wrote:

In D151271#4366595, @everythingfunctional wrote:

A program that did this would be invalid. Wouldn't you want it to crash, rather than silently do something else?

Please note that the there is no definition/redefinition of i inside sub2.

Oops, my response above didn't notice this point. If sub2 doesn't undefine or redefine i, then why would there be a copy-out on the indicated line?

[vzakhari]

Thank you for the quotes, @everythingfunctional! I am sorry if my description is confusing. I tried to use word implicit to denote that the program itself is not trying to define/redefine the constant array expression entity. It is the compiler that tries to write into it implicitly: sub is passing non-contiguous array section of i to sub2, but sub2 is not trying to write to its dummy argument i; at the same time, the compiler has no idea that the actual argument associated with sub's i is non-definable, and it produces a copy-out after a call to sub2 assuming that the actual i(3::2) can be modified inside sub2.

[everythingfunctional]

Aha. I see where the difficulty lies now, but isn't this approach causing all possible attempts to define an undefinable entity to just be silently ignored? Or is there some comparison done at the completion of a call to detect whether the temporary copy was (re/un)defined? If not I think something to that effect should be added (possibly conditioned on certain compiler flags for run-time checking).

[vzakhari]

In D151271#4368702, @everythingfunctional wrote:

Aha. I see where the difficulty lies now, but isn't this approach causing all possible attempts to define an undefinable entity to just be silently ignored? Or is there some comparison done at the completion of a call to detect whether the temporary copy was (re/un)defined? If not I think something to that effect should be added (possibly conditioned on certain compiler flags for run-time checking).

isn't this approach causing all possible attempts to define an undefinable entity to just be silently ignored?

Yes, I think this statement is correct. I believe scalar literal actual arguments have the same behavior as well.

Or is there some comparison done at the completion of a call to detect whether the temporary copy was (re/un)defined?

No, there is currently no checking of this kind. I guess we could have added "has been modified" per-element check in the copy-out code and avoided the writes into the actual argument after the call to sub2 (i.e. inside sub we would compare values from i with values from the copy of i(3::2)) - with this implementation, then, we can just start passing the constant array literal reference to sub: if there are no modifications (in a conformant program), then there will be no writes to the readonly memory; if there is a modification to the copy of i(3::2), then a write to the readonly memory will fail. This sounds doable, but requires memcmp in the Assign runtime before doing the copy. It makes sense to do such a check under an option.

Please note that the same code currently works with the non-HLFIR path, so this commit is just trying to address a correctness issue on the HLFIR path. I would like to proceed with this commit, if you do not mind. I will create a github issue for the runtime check feature. Does it sound acceptable to you?

[jeanPerier]

If not I think something to that effect should be added (possibly conditioned on certain compiler flags for run-time checking).

Makes some sense to me. I think those comparisons would definitely have to be done under some compiler flag (they are not constant time checks).
We have not moved to implementing runtime checks "explicitly" yet (by that I mean that the runtime implementation have a lot of checks that may catch user errors, but we do not call the runtime for the sole purpose of doing checks).
I created a task: https://github.com/orgs/llvm/projects/12/views/2?pane=issue&itemId=29048733

Patch LGTM, thank you Slava!

An alternative could be to have the copy-out "conditionally" rewriting the elements back (by comparing each element for change before copying back). That way, read only memory could still be passed (and sanity check would obtained for free in those case, but not for the cases where the argument is an expression that must be allocated dynamically). That is more involving, and this would penalize the copyout a bit, so I am not sure it is worth it. I am just recording my idea here for the record.

[vzakhari]

Thank you for creating the task and the review, Jean!
@everythingfunctional, thank you for the useful discussion and your attention to the details!

[everythingfunctional]

I would like to proceed with this commit, if you do not mind.  I will create a github issue for the runtime check feature.  Does it sound acceptable to you?

That's perfectly fine. Thanks.

Just for thought, I would only implement the memcmp check at the place the temporary for the constant is created. That way you don't penalise every copy-out operation (unless you think the memcmp can be done faster than the memcopy in some cases).

[vzakhari]

Just for thought, I would only implement the memcmp check at the place the temporary for the constant is created.

Unfortunately, there is no currently a way to detect whethe the temporary copy is created for a constant. The temporary copy and the copy-in/copy-out code are created inside sub around the call to sub2, and at this point i is just a regular dummy argument.

[everythingfunctional]

In D151271#4368998, @vzakhari wrote:

Just for thought, I would only implement the memcmp check at the place the temporary for the constant is created.

Unfortunately, there is no currently a way to detect whethe the temporary copy is created for a constant. The temporary copy and the copy-in/copy-out code are created inside sub around the call to sub2, and at this point i is just a regular dummy argument.

I thought this patch was creating the copy of the constant at the call to sub. You'd do a memcmp on return from sub and error terminate if it was different. No need to do memcmp anywhere down the call stack.

[vzakhari]

In D151271#4369453, @everythingfunctional wrote:

In D151271#4368998, @vzakhari wrote:

Just for thought, I would only implement the memcmp check at the place the temporary for the constant is created.

Unfortunately, there is no currently a way to detect whethe the temporary copy is created for a constant. The temporary copy and the copy-in/copy-out code are created inside sub around the call to sub2, and at this point i is just a regular dummy argument.

I thought this patch was creating the copy of the constant at the call to sub. You'd do a memcmp on return from sub and error terminate if it was different. No need to do memcmp anywhere down the call stack.

Oh, I get it now. Thanks!

flang/include/flang/Optimizer/Builder/HLFIRTools.h

getMaybeDereferencedVariableInterface() const {
     return base.getDefiningOp<fir::FortranVariableOpInterface>();
   }
 
   bool isOptional() const {
     auto varIface = getIfVariableInterface();
     return varIface ? varIface.isOptional() : false;
   }
 
+  bool isParameter() const {
+    auto varIface = getIfVariableInterface();
+    return varIface ? varIface.isParameter() : false;
+  }
+
   // Get the entity as an mlir SSA value containing all the shape, type
   // parameters and dynamic shape information.
   mlir::Value getBase() const { return *this; }
 
   // Get the entity as a FIR base. This may not carry the shape and type
   // parameters information, and even when it is a box with shape information.
   // it will not contain the local lower bounds of the entity. This should
   // be used with care when generating FIR code that does not need this

flang/include/flang/Optimizer/Dialect/FortranVariableInterface.td

let extraClassDeclaration = [{
     }
 
     /// Is this variable represented as the value or address of a fir.box or
     /// fir.class?
     bool isBox() {
       return fir::isBoxAddressOrValue(getBase().getType());
     }
 
+    /// Is this variable a Fortran parameter?
+    bool isParameter() {
+      auto attrs = getFortranAttrs();
+      return attrs && bitEnumContainsAny(*attrs,
+                        fir::FortranVariableFlagsEnum::parameter);
+    }
+
     /// Interface verifier imlementation for declare operations.
     mlir::LogicalResult verifyDeclareLikeOpImpl(mlir::Value memRef);
 
   }];
 
   let cppNamespace = "fir";
 
 }
 
 #endif  // FORTRANVARIABLEINTERFACE

flang/lib/Lower/ConvertCall.cpp

if (mustSetDynamicTypeToDummyType) {
       // Note: this is important to do this before any copy-in or copy so
       // that the dummy is contiguous according to the dummy type.
       mlir::Type boxType =
           fir::BoxType::get(hlfir::getFortranElementOrSequenceType(dummyType));
       entity = hlfir::Entity{builder.create<fir::ReboxOp>(
           loc, boxType, entity, /*shape=*/mlir::Value{},
           /*slice=*/mlir::Value{})};
     }
-    if (arg.hasValueAttribute()) {
+    if (arg.hasValueAttribute() ||
+        // Constant expressions might be lowered as variables with
+        // 'parameter' attribute. Even though the constant expressions
+        // are not definable and explicit assignments to them are not
+        // possible, we have to create a temporary copies when we pass
+        // them down the call stack.
+        entity.isParameter()) {
       // Make a copy in a temporary.
       auto copy = builder.create<hlfir::AsExprOp>(loc, entity);
       hlfir::AssociateOp associate = hlfir::genAssociateExpr(
           loc, builder, hlfir::Entity{copy}, dummyType, "adapt.valuebyref");
       entity = hlfir::Entity{associate.getBase()};
       // Register the temporary destruction after the call.
       preparedDummy.setExprAssociateCleanUp(
           associate.getFirBase(), associate.getMustFreeStrorageFlag());

flang/test/Lower/HLFIR/calls-constant-expr-arg.f90

+! RUN: bbc -emit-fir -hlfir -o - %s | FileCheck %s
+
+! Test that the constant array expression actual argument
+! is placed into a temporary inside 'test' subroutine before
+! the call to 'sub'. This is required because the compiler-generated
+! copy-out inside 'sub' after the call to 'sub2' will write
+! into the dummy argument 'i'. If the constant array expression is passed
+! directly byref to 'sub', the copy-out will try to modify readonly memory.
+subroutine sub(i, n)
+  interface
+     subroutine sub2(i)
+       integer :: i(*)
+     end subroutine sub2
+  end interface
+  integer :: i(n)
+  call sub2(i(3::2))
+end subroutine sub
+! CHECK-LABEL:   func.func @_QPsub(
+! CHECK-SAME:                      %[[VAL_0:.*]]: !fir.ref<!fir.array<?xi32>> {fir.bindc_name = "i"},
+! CHECK-SAME:                      %[[VAL_1:.*]]: !fir.ref<i32> {fir.bindc_name = "n"}) {
+! CHECK:           %[[VAL_2:.*]]:2 = hlfir.declare %[[VAL_1]] {uniq_name = "_QFsubEn"} : (!fir.ref<i32>) -> (!fir.ref<i32>, !fir.ref<i32>)
+! CHECK:           %[[VAL_3:.*]] = fir.load %[[VAL_2]]#0 : !fir.ref<i32>
+! CHECK:           %[[VAL_4:.*]] = fir.convert %[[VAL_3]] : (i32) -> i64
+! CHECK:           %[[VAL_5:.*]] = fir.convert %[[VAL_4]] : (i64) -> index
+! CHECK:           %[[VAL_6:.*]] = arith.constant 0 : index
+! CHECK:           %[[VAL_7:.*]] = arith.cmpi sgt, %[[VAL_5]], %[[VAL_6]] : index
+! CHECK:           %[[VAL_8:.*]] = arith.select %[[VAL_7]], %[[VAL_5]], %[[VAL_6]] : index
+! CHECK:           %[[VAL_9:.*]] = fir.shape %[[VAL_8]] : (index) -> !fir.shape<1>
+! CHECK:           %[[VAL_10:.*]]:2 = hlfir.declare %[[VAL_0]](%[[VAL_9]]) {uniq_name = "_QFsubEi"} : (!fir.ref<!fir.array<?xi32>>, !fir.shape<1>) -> (!fir.box<!fir.array<?xi32>>, !fir.ref<!fir.array<?xi32>>)
+! CHECK:           %[[VAL_11:.*]] = arith.constant 3 : index
+! CHECK:           %[[VAL_12:.*]] = arith.constant 2 : index
+! CHECK:           %[[VAL_13:.*]] = arith.constant 0 : index
+! CHECK:           %[[VAL_14:.*]] = arith.subi %[[VAL_8]], %[[VAL_11]] : index
+! CHECK:           %[[VAL_15:.*]] = arith.addi %[[VAL_14]], %[[VAL_12]] : index
+! CHECK:           %[[VAL_16:.*]] = arith.divsi %[[VAL_15]], %[[VAL_12]] : index
+! CHECK:           %[[VAL_17:.*]] = arith.cmpi sgt, %[[VAL_16]], %[[VAL_13]] : index
+! CHECK:           %[[VAL_18:.*]] = arith.select %[[VAL_17]], %[[VAL_16]], %[[VAL_13]] : index
+! CHECK:           %[[VAL_19:.*]] = fir.shape %[[VAL_18]] : (index) -> !fir.shape<1>
+! CHECK:           %[[VAL_20:.*]] = hlfir.designate %[[VAL_10]]#0 (%[[VAL_11]]:%[[VAL_8]]:%[[VAL_12]])  shape %[[VAL_19]] : (!fir.box<!fir.array<?xi32>>, index, index, index, !fir.shape<1>) -> !fir.box<!fir.array<?xi32>>
+! CHECK:           %[[VAL_21:.*]]:2 = hlfir.copy_in %[[VAL_20]] : (!fir.box<!fir.array<?xi32>>) -> (!fir.box<!fir.array<?xi32>>, i1)
+! CHECK:           %[[VAL_22:.*]] = fir.box_addr %[[VAL_21]]#0 : (!fir.box<!fir.array<?xi32>>) -> !fir.ref<!fir.array<?xi32>>
+! CHECK:           fir.call @_QPsub2(%[[VAL_22]]) fastmath<contract> : (!fir.ref<!fir.array<?xi32>>) -> ()
+! CHECK:           hlfir.copy_out %[[VAL_21]]#0, %[[VAL_21]]#1 to %[[VAL_20]] : (!fir.box<!fir.array<?xi32>>, i1, !fir.box<!fir.array<?xi32>>) -> ()
+! CHECK:           return
+! CHECK:         }
+
+subroutine test
+  call sub((/1,2,3,4,5/), 5)
+end subroutine test
+! CHECK-LABEL:   func.func @_QPtest() {
+! CHECK:           %[[VAL_0:.*]] = fir.address_of(@_QQro.5xi4.0) : !fir.ref<!fir.array<5xi32>>
+! CHECK:           %[[VAL_1:.*]] = arith.constant 5 : index
+! CHECK:           %[[VAL_2:.*]] = fir.shape %[[VAL_1]] : (index) -> !fir.shape<1>
+! CHECK:           %[[VAL_3:.*]]:2 = hlfir.declare %[[VAL_0]](%[[VAL_2]]) {fortran_attrs = #fir.var_attrs<parameter>, uniq_name = "_QQro.5xi4.0"} : (!fir.ref<!fir.array<5xi32>>, !fir.shape<1>) -> (!fir.ref<!fir.array<5xi32>>, !fir.ref<!fir.array<5xi32>>)
+! CHECK:           %[[VAL_4:.*]] = arith.constant 5 : i32
+! CHECK:           %[[VAL_5:.*]] = hlfir.as_expr %[[VAL_3]]#0 : (!fir.ref<!fir.array<5xi32>>) -> !hlfir.expr<5xi32>
+! CHECK:           %[[VAL_6:.*]]:3 = hlfir.associate %[[VAL_5]](%[[VAL_2]]) {uniq_name = "adapt.valuebyref"} : (!hlfir.expr<5xi32>, !fir.shape<1>) -> (!fir.ref<!fir.array<5xi32>>, !fir.ref<!fir.array<5xi32>>, i1)
+! CHECK:           %[[VAL_7:.*]]:3 = hlfir.associate %[[VAL_4]] {uniq_name = "adapt.valuebyref"} : (i32) -> (!fir.ref<i32>, !fir.ref<i32>, i1)
+! CHECK:           %[[VAL_8:.*]] = fir.convert %[[VAL_6]]#1 : (!fir.ref<!fir.array<5xi32>>) -> !fir.ref<!fir.array<?xi32>>
+! CHECK:           fir.call @_QPsub(%[[VAL_8]], %[[VAL_7]]#1) fastmath<contract> : (!fir.ref<!fir.array<?xi32>>, !fir.ref<i32>) -> ()
+! CHECK:           hlfir.end_associate %[[VAL_6]]#1, %[[VAL_6]]#2 : !fir.ref<!fir.array<5xi32>>, i1
+! CHECK:           hlfir.end_associate %[[VAL_7]]#1, %[[VAL_7]]#2 : !fir.ref<i32>, i1
+! CHECK:           return
+! CHECK:         }