This is an archive of the discontinued LLVM Phabricator instance.

[flang] Support lowering of intrinsic module procedure `c_loc`
ClosedPublic

Authored by peixin on Jul 13 2022, 9:36 AM.

Details

Summary

As Fortran 2018 18.2.3.6, the intrinsic c_loc(x) gets the C address
of argument x. It returns the scalar of type C_PTR. As defined in
iso_c_binding in flang/module/__fortran_builtins.f90, C_PTR is the
derived type with only one component of integer 64.

This supports the lowering of intrinsic module procedure c_loc by
converting the address of argument into integer 64, where the argument
is lowered as Box and the address is generated using fir.box_addr.

The lowering of intrinsic c_funloc has the similar characteristic and
will be supported later.

The execution tests for various data types are in issue
https://github.com/llvm/llvm-project/issues/56552.

Diff Detail

Event Timeline

peixin created this revision.Jul 13 2022, 9:36 AM
Herald added a project: Restricted Project. · View Herald Transcript
peixin requested review of this revision.Jul 13 2022, 9:36 AM
clementval added inline comments.Jul 13 2022, 9:41 AM
flang/include/flang/Optimizer/Dialect/FIROps.td
2861 ↗(On Diff #444308)

Shouldn't it be ptrtoint?

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2785 ↗(On Diff #444308)

Do you really need it?

peixin added a comment.EditedJul 13 2022, 9:41 AM

This is verified as follows:

$ cat c_print.c
#include <stdio.h>
void c_print_addr_(char **p) {
  printf("C Address of i = %d\n", p);
}
$ cat main.f90
PROGRAM PROG_FC_HAVE_F2003_REQUIREMENTS
  USE iso_c_binding
  IMPLICIT NONE
  TYPE(C_PTR) :: ptr
  TYPE(C_FUNPTR) :: funptr
  CHARACTER(LEN=80, KIND=c_char), TARGET :: ichr
  ptr = C_LOC(ichr(1:1))
  print *, ptr
  call c_print_addr(ichr(1:1)) 
END PROGRAM PROG_FC_HAVE_F2003_REQUIREMENTS
$ flang-new main.f90 -c
$ clang c_print.c -c
$ flang-new -flang-experimental-exec main.o c_print.o
$ a.out
C Address of i = 5293200
 5293200
flang/test/Lower/Intrinsics/c_loc.f90
38

For this case, fir::getBase(args[0]) is %54 = "fir.load"(%52) : (!fir.ref<i32>) -> i32. I am not sure if it is one bug for lowering of intrinsics. So, I add TODO for now.

peixin updated this revision to Diff 444314.Jul 13 2022, 9:44 AM
peixin added inline comments.
flang/include/flang/Optimizer/Dialect/FIROps.td
2861 ↗(On Diff #444308)

Thanks. Fixed.

flang/lib/Optimizer/CodeGen/CodeGen.cpp
2785 ↗(On Diff #444308)

It is not necessary. Fixed.

peixin planned changes to this revision.Jul 15 2022, 1:18 AM
peixin added inline comments.
flang/test/Lower/Intrinsics/c_loc.f90
38

This is because c_loc is no taken as intrinsic. Instead, it is intrinsic module procedure. The argument lowering rules are not found for intrinsic module procedures, so it uses "val" instread of "addr". Something can be shared with intrinsic (Reuse IntrinsicArgumentLoweringRules with some comment). Will prepare a new one for discussion.

peixin updated this revision to Diff 445079.Jul 15 2022, 11:42 AM
peixin retitled this revision from [flang] Support lowering and codegen of intrinsic `c_loc` to [flang] Support lowering and codegen of intrinsic module procedure `c_loc`.
peixin edited the summary of this revision. (Show Details)

Update the approach.

peixin updated this revision to Diff 445178.Jul 15 2022, 8:47 PM

Rebase to fix the bot fail.

Do we need the new fir.ptrtoint operation? fir.convert can generate ptrtoint llvm instructions if the source is a pointer and destination is an int. https://github.com/llvm/llvm-project/blob/74f6672e59d2cc9fdc95abfd28f527f00c808c07/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L876

peixin updated this revision to Diff 445204.Jul 16 2022, 1:12 AM

Thanks @kiranchandramohan for the notice. Reuse the fir::ConvertOp for address to integer conversion.

peixin updated this revision to Diff 445206.Jul 16 2022, 1:15 AM
peixin edited the summary of this revision. (Show Details)

Fix one typo.

peixin retitled this revision from [flang] Support lowering and codegen of intrinsic module procedure `c_loc` to [flang] Support lowering of intrinsic module procedure `c_loc`.Jul 17 2022, 2:16 AM
peixin edited the summary of this revision. (Show Details)

Do we need the new fir.ptrtoint operation? fir.convert can generate ptrtoint llvm instructions if the source is a pointer and destination is an int. https://github.com/llvm/llvm-project/blob/74f6672e59d2cc9fdc95abfd28f527f00c808c07/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L876

+1. I think from now on we need an RFC if we introduce new FIR operations.

Do we need the new fir.ptrtoint operation? fir.convert can generate ptrtoint llvm instructions if the source is a pointer and destination is an int. https://github.com/llvm/llvm-project/blob/74f6672e59d2cc9fdc95abfd28f527f00c808c07/flang/lib/Optimizer/CodeGen/CodeGen.cpp#L876

+1. I think from now on we need an RFC if we introduce new FIR operations.

Should the description of FIR ConvertOp be changed? Its usage in CodeGen is beyond its description. Here is its description:

def fir_ConvertOp : fir_OneResultOp<"convert", [NoSideEffect]> {
  let summary = "encapsulates all Fortran scalar type conversions";

  let description = [{
    Generalized type conversion. Convert the ssa value from type T to type U.
    Not all pairs of types have conversions. When types T and U are the same
    type, this instruction is a NOP and may be folded away.

    ```mlir
      %v = ... : i64
      %w = fir.convert %v : (i64) -> i32
  The example truncates the value `%v` from an i64 to an i32.
}];
jeanPerier requested changes to this revision.Jul 27 2022, 6:50 AM
jeanPerier added a subscriber: klausler.

I do not think you need to add the BaseAddr case: you could simply lower all arguments to asBox and generate a fir.box_addr to get the address.
All variables (except variables that are designator with vector subscripts(*) ) can be lowered to a fir.box without making any copies. fir.box_addr will get you the base address in a robust way.

Now, the fir.embox + fir.box_addr may annoy you in the IR, especially with scalars where this is basically a no-op, and going via the embox looks stupid, but the good thing is that there is a canonicalizer pass that will get rid of it:

See:

func.func @test(%arg0: !fir.ref<!fir.array<10xf32>> ) {
  %0 = fir.embox %arg0 : (!fir.ref<!fir.array<10xf32>>) -> !fir.box<!fir.array<10xf32>>
  %1 = fir.box_addr %0 : (!fir.box<!fir.array<10xf32>>) -> !fir.ref<!fir.array<10xf32>>
  fir.call @bar(%1) : (!fir.ref<!fir.array<10xf32>>) -> ()
  return
}
func.func private @bar(!fir.ref<!fir.array<10xf32>> ) -> ()

fir-opt --canonicalize output:

func.func @test(%arg0: !fir.ref<!fir.array<10xf32>>) {
  fir.call @bar(%arg0) : (!fir.ref<!fir.array<10xf32>>) -> ()
  return
}
func.func private @bar(!fir.ref<!fir.array<10xf32>>)

So I think relying on asBox + fir.box_addr will make lowering a lot simpler an robust at no extra runtime cost for C_LOC lowering.

(*) I think having a variable that is designator with vector subscript in C_LOC would be most likely illegal: it does not have the "Target" attribute (it cannot be pointed at in a pointer assignment). Ifort and gfortran are not enforcing this, but xlf and NAG are. @klausler, do you agree we should forbid designator with vector subscripts in C_LOC on the motive that they cannot possibly be targets ?

flang/lib/Lower/ConvertExpr.cpp
2296 ↗(On Diff #445206)

prepareActualToBaseAddressLike is inserting copy-in when the arguments are not simply contiguous when passing to explicit shape arguments, we do not need/want that here: we should pass the base address and not deal with the case where the argument is not contiguous (the standard says it must be contiguous, although that is something that could only be checked at compile time, and I have a feeling some programmers out there might rely on getting the base first element address, whether the argument is contiguous or not).

flang/test/Lower/Intrinsics/c_loc.f90
14

Please only add checks relevant to what you are testing here and below. We do not care much about the allocation/assignment here, it is actually a bit confusing since it looks like c_loc lowering is doing a lot of copies...

314

I am actually surprised this case and some other in your file are accepted by semantics. According to the standard about c_loc (18.2.3.6 point 3): X shall have either the POINTER or TARGET attribute. I cannot find an extension in https://github.com/llvm/llvm-project/blob/main/flang/docs/Extensions.md.

@klausler, we want to enforce this, right ?

This revision now requires changes to proceed.Jul 27 2022, 6:50 AM

Thanks @jeanPerier for the detailed explanations. Let me take a try with asBox as suggested.

flang/test/Lower/Intrinsics/c_loc.f90
314

Like what I said in the summary, gfortran does not support allocatable variables or variables without pointer/target attribute and emits one semantic error. ifort support them.

Currently, there is only semantic check of implicit interface for c_loc (https://github.com/llvm/llvm-project/blob/1f5144cdbbab3b86795903a10180c85d7e586e93/flang/lib/Semantics/check-call.cpp#L27-L49). I also filed one issue for assumed type and derived type with kind type paramter in c_loc (https://github.com/llvm/llvm-project/issues/56553).

Should the semantic checks for intrinsics be separated from CheckImplicitInterfaceArg to handle the above cases? Or some better ideas?

I think having a variable that is designator with vector subscript in C_LOC would be most likely illegal: it does not have the "Target" attribute (it cannot be pointed at in a pointer assignment). Ifort and gfortran are not enforcing this, but xlf and NAG are. @klausler, do you agree we should forbid designator with vector subscripts in C_LOC on the motive that they cannot possibly be targets ?

The standard does not exclude variables with vector subscripts from appearing as C_LOC arguments, but since they clearly can't be targets, we should disallow them. The standard does disallow such variables from being data-targets in pointer assignments.

peixin updated this revision to Diff 448505.Jul 28 2022, 8:31 PM
peixin edited the summary of this revision. (Show Details)

Address the comments:

  1. Use asBox for the argument.
  2. Remove the unused file check IRs as suggested.
  3. Remove the test cases without pointer/target attribute, which may should have semantic error. For now, I am not sure since the standard also says "if it is allocatable, it shall be allocated. If it is a pointer, it shall be associated.". This seems to be contradictory. This can be delayed until supporting the semantic checks for c_loc.

and I have a feeling some programmers out there might rely on getting the base first element address, whether the argument is contiguous or not).

Agree.

Thanks, that looks good to me, just one question about the TODO and a related suggestion.

flang/lib/Lower/IntrinsicCall.cpp
2398–2407

I do not fully understand why the POINTER case is left TODO, shouldn't it be handled in the same way ?

If the issue id to get the right memory type, I suggest you use BoxValue::getMemTy():

const auto*box = args[0].getBoxOf<fir::BoxValue>();
assert(box && "c_loc arg must have been lowered to a fir.box");
mlir::Value argAddr = builder.create<fir::BoxAddrOp>(loc, box->getMemTy(), fir::getBase(*box)));

This will ensure the fir.heap/fir.ptr aspect are kept in the BoxAddrOp result.

peixin updated this revision to Diff 448662.Jul 29 2022, 9:25 AM

Use BoxValue::getMemTy() as suggested.

I do not fully understand why the POINTER case is left TODO, shouldn't it be handled in the same way ?

The execution of the following case is:

$ cat a.f90 
program main
  use iso_c_binding
  type(c_ptr) :: ptr
  integer, pointer :: a(:)
  allocate(a(10))
  a = 10
  ptr = c_loc(a)
  print *, "Fortran data = ", a(1)
  print *, "Fortran address = ", ptr
  call c_print_addr(a)
end
$ cat b.c 
#include <stdio.h>

void c_print_addr_(int ***p) {
  printf("C data = %d\n", *p);
  printf("C Address = %d\n", p);
}
$ flang-new a.f90 -c
$ clang b.c -c -w
$ flang-new -flang-experimental-exec a.o b.o
$ ./a.out
C data = 10
C Address = 14530256
 Fortran data =  10
 Fortran address =  14451360

It seems the problem is not lowering of c_loc. The generated FIR for a.f90 is as follows:

%0 = fir.address_of(@_QFEa) : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>
%15 = fir.load %0 : !fir.ref<!fir.box<!fir.ptr<!fir.array<?xi32>>>>
%16 = fir.alloca !fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>
%17 = fir.box_addr %15 : (!fir.box<!fir.ptr<!fir.array<?xi32>>>) -> !fir.ptr<!fir.array<?xi32>>
%18 = fir.convert %17 : (!fir.ptr<!fir.array<?xi32>>) -> i64
%19 = fir.field_index __address, !fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>
%20 = fir.coordinate_of %16, %19 : (!fir.ref<!fir.type<_QM__fortran_builtinsT__builtin_c_ptr{__address:i64}>>, !fir.field) -> !fir.ref<i64>
fir.store %18 to %20 : !fir.ref<i64>
%29 = fir.allocmem !fir.array<?xi32>, %28#1 {uniq_name = ".copyinout"}
%36 = fir.convert %29 : (!fir.heap<!fir.array<?xi32>>) -> !fir.ref<!fir.array<?xi32>>
fir.call @_QPc_print_addr(%36) : (!fir.ref<!fir.array<?xi32>>) -> ()

Currently, I have no idea how to fix this problem. It seems that the lowering of passing pointer array as argument should be fixed.

Currently, I have no idea how to fix this problem. It seems that the lowering of passing pointer array as argument should be fixed.

Yes, that has nothing to do with c_loc lowering indeed. What you are seeing is the copy-in/copy-out of the pointer array, that may be non contiguous, and is passed to an explicit shape array (according to the implicit interface). F18 lowering currently does not make the copy-in/copy-out conditional at runtime. The runtime to check contiguity was added and should be used to make copy-in/copy-out conditional at runtime on the actual contiguity.

Technically, I think the expectations of your program that the array address inside c_print_addr be the same as its argument is wrong because its dummy argument does not have the target attribute, and even if it had, it should be an assumed shape with the CONTIGUOUS attribute, or the actual argument should be simply contiguous (contiguity that can be deduced at compile time) which is not the case here. Fortran 2018 section 15.5.2.4 point 8 and 9 do not guarantee the address will be what you expect in those case, although all compiler will yield the expected result for obvious performance incentive (why making an array copy when we do not need to ?), and this should be updated in lowering at some point.

Anyway, I do not think c_loc deserve a TODO because of that for pointers, this is unrelated and C_LOC works (you can verify this by doing c_print_addr(a(lbound(a,1))) instead of c_print_addr(a)).

peixin added a comment.Aug 1 2022, 1:37 AM

Technically, I think the expectations of your program that the array address inside c_print_addr be the same as its argument is wrong because its dummy argument does not have the target attribute, and even if it had, it should be an assumed shape with the CONTIGUOUS attribute, or the actual argument should be simply contiguous (contiguity that can be deduced at compile time) which is not the case here. Fortran 2018 section 15.5.2.4 point 8 and 9 do not guarantee the address will be what you expect in those case, although all compiler will yield the expected result for obvious performance incentive (why making an array copy when we do not need to ?), and this should be updated in lowering at some point.

Got it. Thanks. Will remove the TODO.

peixin updated this revision to Diff 448983.Aug 1 2022, 3:57 AM
peixin edited the summary of this revision. (Show Details)

Remove the TODO for pointer array and add the test case for it.

jeanPerier accepted this revision.Aug 2 2022, 12:38 AM

Looks good to me, thanks !

This revision is now accepted and ready to land.Aug 2 2022, 12:38 AM