[Flang][OpenMP][MLIR] An mlir transformation pass for marking FuncOp's implicitly called from TargetOp's and declare target marked FuncOp's as implicitly declare target

[agozillon]

Summary

This pass will mark functions called from TargetOp's
and declare target functions as implicitly declare
target by adding the MLIR declare target attribute
directly to the function.

This pass executes after the initial lowering of Fortran's PFT
to MLIR (FIR/OMP+Arith etc.) and is one of a series of passes
that aim to clean up the MLIR for offloading (seperate passes
in different patches, one for early outlining, another for declare
target function filtering).

The pass is an upgraded version of the prior MLIR Opt
pass that I created that did the same thing. In this case
better handling of recursive cases, updated to work with
the interface and the ability to work with TargetOp's.

The semantic pass equivalent of this can be found in:
https://reviews.llvm.org/D150323

So we can choose to go either direction, however, this
pass is simpler to maintain and understand and having
it as an transformation pass keeps it consistent with the
other transformation passes that deal with OpenMP
function and target op processing.

[skatrak]

I like this approach, and I think it is a more manageable approach than semantics, but it looks to me like this pass could be moved outside of the Flang project and into the OpenMP MLIR dialect as a transformation pass. I may be wrong, so please correct me if that's the case, but I understand that implicit declare target propagation should be common to Fortran and C/C++ OpenMP. So it makes sense to me to have the pass defined where it can be reused.

So I would propose, rather than defining the pass in Flang and propagating declare target information through fir.call ops, building on top of D154194 to create the pass as part of the OpenMP dialect (D147641 shows an example of what I mean) and to propagate declare target through func.call ops. The implementation of the pass itself I think would remain mostly unchanged except from swapping fir.call with func.call, and then it would just be a matter of moving some files around. I think this way the offloading-related passes (implicit declare target, target region outlining and host/device function filtering for now) would be implemented in a consistent and reusable way.

I may be missing some other considerations, so I'd be interested to hear any other opinions you or others may have.

flang/lib/Optimizer/Transforms/OMPCaptureImplicitDeclTar.cpp

79

In the case of a regular target region (omp.target inside of a host function) this is correct. However, a reverse-offloading target region (omp.target inside of a device function and with the 'ancestor' clause set) should instead mark functions called inside as host functions.

Currently there is no support for reverse offloading, so maybe it's not important to already mark functions properly here, but at least it would be good to acknowledge this known limitation in a TODO comment.

[kiranchandramohan]

In D154247#4470828, @skatrak wrote:

I like this approach, and I think it is a more manageable approach than semantics, but it looks to me like this pass could be moved outside of the Flang project and into the OpenMP MLIR dialect as a transformation pass. I may be wrong, so please correct me if that's the case, but I understand that implicit declare target propagation should be common to Fortran and C/C++ OpenMP. So it makes sense to me to have the pass defined where it can be reused.

So I would propose, rather than defining the pass in Flang and propagating declare target information through fir.call ops, building on top of D154194 to create the pass as part of the OpenMP dialect (D147641 shows an example of what I mean) and to propagate declare target through func.call ops. The implementation of the pass itself I think would remain mostly unchanged except from swapping fir.call with func.call, and then it would just be a matter of moving some files around. I think this way the offloading-related passes (implicit declare target, target region outlining and host/device function filtering for now) would be implemented in a consistent and reusable way.

I may be missing some other considerations, so I'd be interested to hear any other opinions you or others may have.

I have not gone through the details. But the function calls in FIR always exist as a fir.call till conversion to llvm.call.

I agree that this is simpler to maintain than the Semantics pass. My preference (not a strong opinion) is to keep this pass in Flang, and be called early on (one of the first passes after lowering) and be looked at as part of the lowering process. Practically, this also gives you a lot of leeway to experiment and make changes. Once we are all clear with the flow then we can think of moving these to the OpenMP Dialect.

[kiranchandramohan]

I have not gone through the details. But the function calls in FIR always exist as a fir.call till conversion to llvm.call.

There is the CallOpInterface that can be used instead if you do not want to rely on a FIR operation.

[agozillon]

In D154247#4470828, @skatrak wrote:

I like this approach, and I think it is a more manageable approach than semantics, but it looks to me like this pass could be moved outside of the Flang project and into the OpenMP MLIR dialect as a transformation pass. I may be wrong, so please correct me if that's the case, but I understand that implicit declare target propagation should be common to Fortran and C/C++ OpenMP. So it makes sense to me to have the pass defined where it can be reused.

So I would propose, rather than defining the pass in Flang and propagating declare target information through fir.call ops, building on top of D154194 to create the pass as part of the OpenMP dialect (D147641 shows an example of what I mean) and to propagate declare target through func.call ops. The implementation of the pass itself I think would remain mostly unchanged except from swapping fir.call with func.call, and then it would just be a matter of moving some files around. I think this way the offloading-related passes (implicit declare target, target region outlining and host/device function filtering for now) would be implemented in a consistent and reusable way.

I may be missing some other considerations, so I'd be interested to hear any other opinions you or others may have.

I would have also preferred to have kept them together as an OpenMP pass, but the pass unfortunately relies on fir.call ops as like @kiranchandramohan says the func.call's aren't a thing in FIR as far as I was aware and I don't believe there is inheritance to dynamically cast between them or infer one is the same as the other, which means it has to sit inside of Flang as FIR isn't part of the MLIR project, we'd need to utilize an interface as @kiranchandramohan suggested (also discussed with @jsjodin but decided it wasn't worth the effort to implement an interface at the moment for this reason alone), however, as @kiranchandramohan mentioned there is a CallOpInterface so perhaps this would work. Although, perhaps there is something I am missing (very likely) and func.call or some other replacement for fir.call other than the interface can be used!

In D154247#4470898, @kiranchandramohan wrote:

In D154247#4470828, @skatrak wrote:

I like this approach, and I think it is a more manageable approach than semantics, but it looks to me like this pass could be moved outside of the Flang project and into the OpenMP MLIR dialect as a transformation pass. I may be wrong, so please correct me if that's the case, but I understand that implicit declare target propagation should be common to Fortran and C/C++ OpenMP. So it makes sense to me to have the pass defined where it can be reused.

So I would propose, rather than defining the pass in Flang and propagating declare target information through fir.call ops, building on top of D154194 to create the pass as part of the OpenMP dialect (D147641 shows an example of what I mean) and to propagate declare target through func.call ops. The implementation of the pass itself I think would remain mostly unchanged except from swapping fir.call with func.call, and then it would just be a matter of moving some files around. I think this way the offloading-related passes (implicit declare target, target region outlining and host/device function filtering for now) would be implemented in a consistent and reusable way.

I may be missing some other considerations, so I'd be interested to hear any other opinions you or others may have.

I have not gone through the details. But the function calls in FIR always exist as a fir.call till conversion to llvm.call.

I agree that this is simpler to maintain than the Semantics pass. My preference (not a strong opinion) is to keep this pass in Flang, and be called early on (one of the first passes after lowering) and be looked at as part of the lowering process. Practically, this also gives you a lot of leeway to experiment and make changes. Once we are all clear with the flow then we can think of moving these to the OpenMP Dialect.

I'm not overly bothered where the opt pass rests at the moment personally (provided I can make it a little more operation agnostic), so I'm happy to change this opt pass to reside in the OpenMP dialect or the FIR dialects depending on the outcome of both (@kiranchandramohan @skatrak) your discussions and if the CallOpInterface yields any success.

I do not think it being part of the OpenMP dialect will prevent it being called early on (it currently happens just before verification, in the same opt pipeline, which I believe is directly after lowering, it is only optional added to the pipeline based on the OpenMP flag being set currently. As far as I am aware this is where the rest will be called as well), but being cautious with testing the flow before making it part of the OpenMP Dialect seems reasonable, although I don't think anything is going to use it in the meantime, at least until/if Clang swaps over to generating MLIR.

flang/lib/Optimizer/Transforms/OMPCaptureImplicitDeclTar.cpp

79

I'll see how difficult this is to add, but it may end up being a TODO!

[skatrak]

In D154247#4470898, @kiranchandramohan wrote:

I have not gone through the details. But the function calls in FIR always exist as a fir.call till conversion to llvm.call.

You are right, I wasn't aware of this. Then, as you say, the alternative to having this pass work as part of the OpenMP dialect would be to use the CallOpInterface rather than searching for func.call.

In D154247#4470898, @kiranchandramohan wrote:

Practically, this also gives you a lot of leeway to experiment and make changes. Once we are all clear with the flow then we can think of moving these to the OpenMP Dialect.

That is a good point. By the same logic, then it would be probably good to also define the passes for target region outlining and host/device function filtering inside of Flang at first. Once all this stabilizes, then we can think of moving these (and potentially other) transformation passes to the dialect. Even then, it would be worthwhile to try implementing these passes without dependencies to the FIR dialect as much as possible, so that the eventual move won't be too involved.

In D154247#4471118, @agozillon wrote:

I'm not overly bothered where the opt pass rests at the moment personally (provided I can make it a little more operation agnostic), so I'm happy to change this opt pass to reside in the OpenMP dialect or the FIR dialects depending on the outcome of both (@kiranchandramohan @skatrak) your discussions and if the CallOpInterface yields any success.

I think it's fine to keep it as part of Flang for now, but trying to use the CallOpInterface if possible, to remove the dependency on the FIR dialect.

In D154247#4471118, @agozillon wrote:

I do not think it being part of the OpenMP dialect will prevent it being called early on (it currently happens just before verification, in the same opt pipeline, which I believe is directly after lowering, it is only optional added to the pipeline based on the OpenMP flag being set currently. As far as I am aware this is where the rest will be called as well), but being cautious with testing the flow before making it part of the OpenMP Dialect seems reasonable, although I don't think anything is going to use it in the meantime, at least until/if Clang swaps over to generating MLIR.

I agree, I think at the moment it's not really that important where the pass is defined because it will be created in the same spots inside of Flang regardless.

[agozillon]

• [Flang][MLIR][OpenMP] Make the pass more generic and remove the use of Fir.CallOps via the CallOpInterface
• [Flang][MLIR][OpenMP] Add comment for future extension for reverse-offloading

[agozillon]

• Add missing newlines at eof

[agozillon]

Added utilisation of the CallOpInterface in the most recent set of commits so it's now no longer dependent on FIR in it's current incarnation. Also added a TODO for the reverse-offloading support, as unfortunately the lowering from the PFT to the Op doesn't handle the device clause for the moment. Otherwise fixed the standard forgetting newlines at the end of new files that I always do (sorry)!

flang/lib/Optimizer/Transforms/OMPCaptureImplicitDeclTar.cpp

79

I've added a TODO comment for the time being, I don't think implementing it will be particularly difficult, the current issue is that there is no lowering of the device clause for the TargetOp so no way to test it unfortunately.

[agozillon]

If it would be possible to get some further review on this it would be greatly appreciated! Thank you very much ahead of time.

[kiranchandramohan]

Nit: Better to refer to this as a transformation pass and not an optimization pass in the title.
Nit: Drop the omp prefix from the test files.
Nit: I think it is better to have tests with 2 spacing and lowercase.

In general (even for c/c++), how does this work for called functions/subroutines that are in a different file? e.g below where module a, module b are in different files.

module a
  use b
contains
  function double() result(x)
    integer :: x
    !$omp declare target
    x = add(10,20)
  end function
end module

module b
contains
  function add(x,y)
    integer :: x, y
    add = x + y
  end function
end module

What is the expected behaviour for operations that might lower to function calls later in the lowering pipeline? A simple example is the math sin function. But there could be others as well.

function mysin(x)
  real :: x
  !$omp declare target
  print *, sin(x)
end function

flang/include/flang/Optimizer/Transforms/Passes.td

301

Prefix the name with OpenMP or OMP.
Would OMPMarkDeclareTargetPass be a better name?

303

[agozillon]

In D154247#4489011, @kiranchandramohan wrote:

Nit: Better to refer to this as a transformation pass and not an optimization pass in the title.
Nit: Drop the omp prefix from the test files.
Nit: I think it is better to have tests with 2 spacing and lowercase.

In general (even for c/c++), how does this work for called functions/subroutines that are in a different file? e.g below where module a, module b are in different files.

module a
  use b
contains
  function double() result(x)
    integer :: x
    !$omp declare target
    x = add(10,20)
  end function
end module

module b
contains
  function add(x,y)
    integer :: x, y
    add = x + y
  end function
end module

In the above case the pass simply does nothing, it will not mark add as declare target as it's in another compilation unit, and the pass won't fail either (from running a test at least). This is specification defined though from my understanding/reading, so it isn't something uncommon:

"If a procedure appears in an enter clause in the same compilation unit in which the definition of the procedure occurs then a device-specific version of the procedure is created for all devices to which the directive of the clause applies.", Section 5.8.4 of specification 5.2

The to clause got renamed to enter after 5.1, so it's essentially an alias. But in this case to get functioning code, the user would I think have to decorate add with its own declare target directive I believe to make it "legal" OpenMP code. Someone with more experience in OpenMP may be able to shed more light or refute my understanding of it though.

The above sort of also hinges on if two seperate modules in two seperate files are considered seperate compilation units, e.g. it's not like it's some C++ header only library (from doing a test, it doesn't appear to be like that though, but I'm fairly new to Fortran).

What is the expected behaviour for operations that might lower to function calls later in the lowering pipeline? A simple example is the math sin function. But there could be others as well.

function mysin(x)
  real :: x
  !$omp declare target
  print *, sin(x)
end function

In this case, it's not going to apply declare target to it unfortunately, as it's not a function or a call, it's I suppose effectively a builtin at this point, until it's lowered. In theory, you could extend the pass to support marking of these I imagine, or look at it as falling under something defined in another compilation unit e.g. some kind of library.

However, in either case, from my current understanding, I believe it's a non-issue. The function filtering pass will execute immediately after this and provided the function it exists in, in this example. mysin survives the culling, it will just be lowered as normal with no other affects at the moment, so the math.sin function would just go through it's regular lowering, and if it happens to spawn a new function, then that's fine, it will not be harmed or go through any other alterations other than regular lowering.

[agozillon]

• Pass name change
• remove omp from test
• rename other tests I've added with similar omp prefix
• tidy up test as suggested

[agozillon]

Applied reviewer nits in the last series of commits and also removed omp- prefix from previous tests I wrote and added the prefix to while I was there, just to keep consistency.

[kiranchandramohan]

However, in either case, from my current understanding, I believe it's a non-issue. The function filtering pass will execute immediately after this and provided the function it exists in, in this example. mysin survives the culling, it will just be lowered as normal with no other affects at the moment, so the math.sin function would just go through it's regular lowering, and if it happens to spawn a new function, then that's fine, it will not be harmed or go through any other alterations other than regular lowering.

When it is written as different passes, there is no guarantee that these passes will be invoked one after the other. In Flang we can probably control this. Would combining the marking and erasing passes be better? Are there any other uses for having these declare target attributes? From your existing reply, i am assuming that is the only use.

Also, the best example for the situation I mentioned previously is the following.

subroutine sb1(x, y, i)
  real :: x, y
  integer(kind=8) :: i
  !$omp declare target
  y = x ** i
end subroutine

func.func @sb1_(%arg0: !fir.ref<f32> {fir.bindc_name = "x"}, %arg1: !fir.ref<f32> {fir.bindc_name = "y"}, %arg2: !fir.ref<i64> {fir.bindc_name = "i"}) attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>} {
  %0 = fir.load %arg0 : !fir.ref<f32>
  %1 = fir.load %arg2 : !fir.ref<i64>
  %2 = call @__mlir_math_fpowi_f32_i64(%0, %1) : (f32, i64) -> f32
  fir.store %2 to %arg1 : !fir.ref<f32>
  return
}

func.func private @__mlir_math_fpowi_f32_i64(%arg0: f32, %arg1: i64) -> f32 attributes {llvm.linkage = #llvm.linkage<linkonce_odr>} {
  %cst = arith.constant 1.000000e+00 : f32
  ...
}

Some comments inline. Looks Good. Please wait for one more acceptance.

flang/lib/Optimizer/Transforms/OMPMarkDeclareTarget.cpp

25 ↗

(On Diff #539616)

Cant the moduleOp be part of the class so that you do not have to pass it around?

26–32 ↗

(On Diff #539616)

Would a set or map be better here?

[agozillon]

In D154247#4496810, @kiranchandramohan wrote:

However, in either case, from my current understanding, I believe it's a non-issue. The function filtering pass will execute immediately after this and provided the function it exists in, in this example. mysin survives the culling, it will just be lowered as normal with no other affects at the moment, so the math.sin function would just go through it's regular lowering, and if it happens to spawn a new function, then that's fine, it will not be harmed or go through any other alterations other than regular lowering.

When it is written as different passes, there is no guarantee that these passes will be invoked one after the other. In Flang we can probably control this. Would combining the marking and erasing passes be better? Are there any other uses for having these declare target attributes? From your existing reply, i am assuming that is the only use.

if they're both passes built to execute on ModuleOp, are they not guaranteed to execute in-order? I have a very minimal understanding of how the pipeline manager runs, but wouldn't one of the passes have to be either added as a nested pass explicitly or implicitly (only seems to happen if a pass executes on a different Op type than the PassManager is given) for them to not run in the provided sequence? I am not against combining them though if that is something we wish to do @skatrak @jsjodin @kiranchandramohan, I'm just not sure it's necessary in the context of them being run out of order, perhaps someone with more MLIR knowledge than me can feed into the discussion. In either case, it would require a bit of a merger of this patch and https://reviews.llvm.org/D147641 or waiting until one is landed and then having the other rebase and do the merging of components on top of it.

For the moment, for functions it's the only use. For global data the attribute has a different use-case, which another patch is up for: https://reviews.llvm.org/D149368

Also, the best example for the situation I mentioned previously is the following.

subroutine sb1(x, y, i)
  real :: x, y
  integer(kind=8) :: i
  !$omp declare target
  y = x ** i
end subroutine

func.func @sb1_(%arg0: !fir.ref<f32> {fir.bindc_name = "x"}, %arg1: !fir.ref<f32> {fir.bindc_name = "y"}, %arg2: !fir.ref<i64> {fir.bindc_name = "i"}) attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>} {
  %0 = fir.load %arg0 : !fir.ref<f32>
  %1 = fir.load %arg2 : !fir.ref<i64>
  %2 = call @__mlir_math_fpowi_f32_i64(%0, %1) : (f32, i64) -> f32
  fir.store %2 to %arg1 : !fir.ref<f32>
  return
}

func.func private @__mlir_math_fpowi_f32_i64(%arg0: f32, %arg1: i64) -> f32 attributes {llvm.linkage = #llvm.linkage<linkonce_odr>} {
  %cst = arith.constant 1.000000e+00 : f32
  ...
}

Thank you for the example! I believe if we did encounter it causing issues in this situation, we could try to move the passes after the math lowering, which would I think be in the FIRToLLVMLowering pass (perhaps I'm wrong on the location though). The ordering may be more difficult to restrict there though as there's a number of math related passes going on. But for the moment, filtering as close to the original lowering as possible is likely the best option I think.

Some comments inline. Looks Good. Please wait for one more acceptance.

Thank you, I'll try to address these and ask if someone else can have a look.

[jsjodin]

In D154247#4496810, @kiranchandramohan wrote:

However, in either case, from my current understanding, I believe it's a non-issue. The function filtering pass will execute immediately after this and provided the function it exists in, in this example. mysin survives the culling, it will just be lowered as normal with no other affects at the moment, so the math.sin function would just go through it's regular lowering, and if it happens to spawn a new function, then that's fine, it will not be harmed or go through any other alterations other than regular lowering.

When it is written as different passes, there is no guarantee that these passes will be invoked one after the other. In Flang we can probably control this. Would combining the marking and erasing passes be better? Are there any other uses for having these declare target attributes? From your existing reply, i am assuming that is the only use.

We can create another patch that implements a pass that invokes the three different passes in the order we want. It seems cleaner to keep the code separate so that it is easier to reason about each transform individually.

[agozillon]

• Pass name change
• remove omp from test
• rename other tests I've added with similar omp prefix
• tidy up test as suggested
• Remove ModuleOp Arg
• Replace vector with set
• Move addPass into same segment as other OpenMP addPass

[agozillon]

Rebased and updated patch based on reviewer comments (use set and remove ModuleOp argument)

[jsjodin]

flang/lib/Optimizer/Transforms/OMPMarkDeclareTarget.cpp

85 ↗

(On Diff #540056)

Can this set be a SmallPtrSet (for Operation *) instead of a StringRef? I am thinking doing mem compare might be expensive.

[agozillon]

• Pass name change
• remove omp from test
• rename other tests I've added with similar omp prefix
• tidy up test as suggested
• Remove ModuleOp Arg
• Replace vector with set
• Move addPass into same segment as other OpenMP addPass
• Use a smallptrset rather than a regualr set

[agozillon]

Update to utilise SmallPtrSet<Operation*> instead of std::set<StringRef>

flang/lib/Optimizer/Transforms/OMPMarkDeclareTarget.cpp

85 ↗

(On Diff #540056)

Thank you for pointing it out, the last commit should address this and use SmallPtrSet!

[jsjodin]

LGTM! Please wait for another acceptance.

[agozillon]

@kiranchandramohan would it be possible to get a 2nd acceptance from you if possible please, if you're happy with the patch in its current state of course! or @skatrak if you wish to give the patch a final look over too see if it's in a state for acceptance.

[skatrak]

Thank you Andrew, LGTM.

In D154247#4497729, @jsjodin wrote:

In D154247#4496810, @kiranchandramohan wrote:

However, in either case, from my current understanding, I believe it's a non-issue. The function filtering pass will execute immediately after this and provided the function it exists in, in this example. mysin survives the culling, it will just be lowered as normal with no other affects at the moment, so the math.sin function would just go through it's regular lowering, and if it happens to spawn a new function, then that's fine, it will not be harmed or go through any other alterations other than regular lowering.

When it is written as different passes, there is no guarantee that these passes will be invoked one after the other. In Flang we can probably control this. Would combining the marking and erasing passes be better? Are there any other uses for having these declare target attributes? From your existing reply, i am assuming that is the only use.

We can create another patch that implements a pass that invokes the three different passes in the order we want. It seems cleaner to keep the code separate so that it is easier to reason about each transform individually.

I agree with this approach. We can define each of these as an independent patch, potentially relying on having one of the others run prior to it, and then make sure that either they all run in their expected order or none of them do by grouping them into a kind of "OpenMP-offloading pipeline" pass (which we can implement as a following patch, if we decide to follow that approach). This pass should run as soon as possible to avoid marking compiler-generated calls like @__mlir_math_fpowi_f32_i64 in the example above.

flang/lib/Optimizer/Transforms/CMakeLists.txt

20

Nit: Seems like there are some leftover spaces there

[agozillon]

In D154247#4500767, @skatrak wrote:

Thank you Andrew, LGTM.

Thank you for the review!

In D154247#4497729, @jsjodin wrote:

In D154247#4496810, @kiranchandramohan wrote:

However, in either case, from my current understanding, I believe it's a non-issue. The function filtering pass will execute immediately after this and provided the function it exists in, in this example. mysin survives the culling, it will just be lowered as normal with no other affects at the moment, so the math.sin function would just go through it's regular lowering, and if it happens to spawn a new function, then that's fine, it will not be harmed or go through any other alterations other than regular lowering.

When it is written as different passes, there is no guarantee that these passes will be invoked one after the other. In Flang we can probably control this. Would combining the marking and erasing passes be better? Are there any other uses for having these declare target attributes? From your existing reply, i am assuming that is the only use.

We can create another patch that implements a pass that invokes the three different passes in the order we want. It seems cleaner to keep the code separate so that it is easier to reason about each transform individually.

I agree with this approach. We can define each of these as an independent patch, potentially relying on having one of the others run prior to it, and then make sure that either they all run in their expected order or none of them do by grouping them into a kind of "OpenMP-offloading pipeline" pass (which we can implement as a following patch, if we decide to follow that approach). This pass should run as soon as possible to avoid marking compiler-generated calls like @__mlir_math_fpowi_f32_i64 in the example above.

This approach sounds good to me.

I'll push this patch on Monday afternoon and fix the nit before I push, provided nobody has any further comments!

[agozillon]

I am going to land this in the next couple of hours now, provided I run into no issues applying it! Thank you all for your reviews and help.

[kiranchandramohan]

The gfortran testsuite is failing after this change. Could you fix immediately or revert until it is fixed?
https://lab.llvm.org/buildbot/#/builders/179/builds/6693
https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/gomp/defaultmap-7.f90

[agozillon]

In D154247#4509624, @kiranchandramohan wrote:

The gfortran testsuite is failing after this change. Could you fix immediately or revert until it is fixed?
https://lab.llvm.org/buildbot/#/builders/179/builds/6693
https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/gomp/defaultmap-7.f90

Hi Kiran, looking into it just now and have an idea of what the problem is, discussing a fix and it should be committed by the end of today.

[agozillon]

In D154247#4509624, @kiranchandramohan wrote:

The gfortran testsuite is failing after this change. Could you fix immediately or revert until it is fixed?
https://lab.llvm.org/buildbot/#/builders/179/builds/6693
https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/gomp/defaultmap-7.f90

It's an interaction between my pass and @skatrak 's pass added in: https://reviews.llvm.org/D147641

I've made a commit/fix for the time being that deactivates the pass for the host, but leaves it enabled for device: https://reviews.llvm.org/rG64f5a7642a05

@skatrak has an idea of how to do the filtering for the host in a way that won't adversely affect things hopefully, so I believe the current plan is for @skatrak to investigate his idea a little further and that should allow us to re-enable filtering for the host. The host filtering is primarily an optimisation/something to allow closer following of the OpenMP specification, it's not a necessity for offloading but it would be nice to have, especially if we aim to implement reverse offloading in the future.

[kiranchandramohan]

In D154247#4512187, @agozillon wrote:

In D154247#4509624, @kiranchandramohan wrote:

The gfortran testsuite is failing after this change. Could you fix immediately or revert until it is fixed?
https://lab.llvm.org/buildbot/#/builders/179/builds/6693
https://github.com/llvm/llvm-test-suite/blob/main/Fortran/gfortran/regression/gomp/defaultmap-7.f90

It's an interaction between my pass and @skatrak 's pass added in: https://reviews.llvm.org/D147641

I've made a commit/fix for the time being that deactivates the pass for the host, but leaves it enabled for device: https://reviews.llvm.org/rG64f5a7642a05

@skatrak has an idea of how to do the filtering for the host in a way that won't adversely affect things hopefully, so I believe the current plan is for @skatrak to investigate his idea a little further and that should allow us to re-enable filtering for the host. The host filtering is primarily an optimisation/something to allow closer following of the OpenMP specification, it's not a necessity for offloading but it would be nice to have, especially if we aim to implement reverse offloading in the future.

Thanks @agozillon for the quick response and the plan for fixing this.

[skatrak]

I have spent some time working on this problem and I just want to ask for your opinions on what the expected behavior is, because I'm not sure. Let's say we have the following application:

subroutine g()
  !$omp declare target to(g) device_type(nohost)
end subroutine device

subroutine f()
  !$omp target
  call g()
  !$omp end target
end subroutine host

I was thinking that g, since it's a device subroutine, should be filtered out when compiling for the host. But code generation for the contents of target regions is also triggered during host compilation, to provide a host fallback implementation. So it seems like it actually would be necessary to keep "nohost" functions also for the host, so that these fallbacks can actually work. If that's the case, then the current solution of only triggering function filtering during device compilation would be correct.

I think this wouldn't impact our ability of implementing reverse offloading later because that just requires function filtering to make sure device functions containing a target region are kept. The only problem could be, during device compilation, the potential calls to host-only functions inside of these reverse offload target regions (if they are removed, the function calls turn into calls to undefined symbols). The patch I am preparing changes the way in which functions are filtered so that undefined references are avoided in that situation.

So would the solution to this be to only filter host-only functions while compiling for the device and tweaking it slightly to avoid calls to undefined references or am I going the wrong way about this?

[agozillon]

In D154247#4514230, @skatrak wrote:

I have spent some time working on this problem and I just want to ask for your opinions on what the expected behavior is, because I'm not sure. Let's say we have the following application:

subroutine g()
  !$omp declare target to(g) device_type(nohost)
end subroutine device

subroutine f()
  !$omp target
  call g()
  !$omp end target
end subroutine host

I was thinking that g, since it's a device subroutine, should be filtered out when compiling for the host. But code generation for the contents of target regions is also triggered during host compilation, to provide a host fallback implementation. So it seems like it actually would be necessary to keep "nohost" functions also for the host, so that these fallbacks can actually work. If that's the case, then the current solution of only triggering function filtering during device compilation would be correct.

I think this wouldn't impact our ability of implementing reverse offloading later because that just requires function filtering to make sure device functions containing a target region are kept. The only problem could be, during device compilation, the potential calls to host-only functions inside of these reverse offload target regions (if they are removed, the function calls turn into calls to undefined symbols). The patch I am preparing changes the way in which functions are filtered so that undefined references are avoided in that situation.

So would the solution to this be to only filter host-only functions while compiling for the device and tweaking it slightly to avoid calls to undefined references or am I going the wrong way about this?

I think that would be correct behavior, as we do need the functions for the fallback.

I suppose you could in theory mark functions needed for fallback as device_type::Any and that'd allow discarding those marked as nohost only, but that's a bit of an excessive and unnecessary optimisation I think, I imagine the cases where a nohost function is not needed in a fallback host implementation is few and far between and something else is likely to clean it up.

[skatrak]

In D154247#4514372, @agozillon wrote:

I think that would be correct behavior, as we do need the functions for the fallback.

I suppose you could in theory mark functions needed for fallback as device_type::Any and that'd allow discarding those marked as nohost only, but that's a bit of an excessive and unnecessary optimisation I think, I imagine the cases where a nohost function is not needed in a fallback host implementation is few and far between and something else is likely to clean it up.

Thank you Andrew for your comments. I just created D155827 to follow this approach and clean up a bit some remaining TODO comments.

flang/include/flang/Optimizer/Transforms/Passes.h

 #include <memory>
 
 namespace mlir {
 class IRMapping;
 class GreedyRewriteConfig;
 class Operation;
 class Pass;
 class Region;
+class ModuleOp;
 } // namespace mlir
 
 namespace fir {
 
 //===----------------------------------------------------------------------===//
 // Passes defined in Passes.td
 //===----------------------------------------------------------------------===//
 

 std::unique_ptr<mlir::Pass>
 createMemoryAllocationPass(bool dynOnHeap, std::size_t maxStackSize);
 std::unique_ptr<mlir::Pass> createAnnotateConstantOperandsPass();
 std::unique_ptr<mlir::Pass> createSimplifyRegionLitePass();
 std::unique_ptr<mlir::Pass> createAlgebraicSimplificationPass();
 std::unique_ptr<mlir::Pass>
 createAlgebraicSimplificationPass(const mlir::GreedyRewriteConfig &config);
 std::unique_ptr<mlir::Pass> createPolymorphicOpConversionPass();
+std::unique_ptr<mlir::OperationPass<mlir::ModuleOp>>
+createCaptureImplicitlyDeclareTargetPass();
 
 // declarative passes
 #define GEN_PASS_REGISTRATION
 #include "flang/Optimizer/Transforms/Passes.h.inc"
 
 } // namespace fir
 
 #endif // FORTRAN_OPTIMIZER_TRANSFORMS_PASSES_H

flang/include/flang/Optimizer/Transforms/Passes.td

let description = [{
     array is an assumed shape array, to optimize for the (often common) case where
     an array has element sized stride. The element sizes stride allows some
     loops to be vectorized as well as other loop optimizations.
   }];
   let constructor = "::fir::createLoopVersioningPass()";
   let dependentDialects = [ "fir::FIROpsDialect" ];
 }
 
+def CaptureImplicitlyDeclareTargetPass
+    : Pass<"capture-implicitly-declare-target", "mlir::ModuleOp"> {
+  let summary = "Marks all functions referenced by a declare target function as declare target";
+  let constructor = "::fir::createCaptureImplicitlyDeclareTargetPass()";
+  let dependentDialects = ["mlir::omp::OpenMPDialect"];
+}
+
 #endif // FLANG_OPTIMIZER_TRANSFORMS_PASSES

flang/lib/Frontend/FrontendActions.cpp

 #include "flang/Frontend/PreprocessorOptions.h"
 #include "flang/Lower/Bridge.h"
 #include "flang/Lower/PFTBuilder.h"
 #include "flang/Lower/Support/Verifier.h"
 #include "flang/Optimizer/Dialect/Support/FIRContext.h"
 #include "flang/Optimizer/Dialect/Support/KindMapping.h"
 #include "flang/Optimizer/Support/InitFIR.h"
 #include "flang/Optimizer/Support/Utils.h"
+#include "flang/Optimizer/Transforms/Passes.h"
 #include "flang/Parser/dump-parse-tree.h"
 #include "flang/Parser/parsing.h"
 #include "flang/Parser/provenance.h"
 #include "flang/Parser/source.h"
 #include "flang/Parser/unparse.h"
 #include "flang/Semantics/runtime-type-info.h"
 #include "flang/Semantics/semantics.h"
 #include "flang/Semantics/unparse-with-symbols.h"
bool CodeGenAction::beginSourceFileAction() {
   // Create a parse tree and lower it to FIR
   Fortran::parser::Program &parseTree{*ci.getParsing().parseTree()};
   lb.lower(parseTree, ci.getInvocation().getSemanticsContext());
 
   // run the default passes.
   mlir::PassManager pm((*mlirModule)->getName(),
                        mlir::OpPassManager::Nesting::Implicit);
   pm.enableVerifier(/*verifyPasses=*/true);
+
+  // Add OpenMP-related passes
+  if (ci.getInvocation().getFrontendOpts().features.IsEnabled(
+          Fortran::common::LanguageFeature::OpenMP)) {
+    pm.addPass(fir::createCaptureImplicitlyDeclareTargetPass());
+  }
+
   pm.addPass(std::make_unique<Fortran::lower::VerifierPass>());
 
   if (mlir::failed(pm.run(*mlirModule))) {
     unsigned diagID = ci.getDiagnostics().getCustomDiagID(
         clang::DiagnosticsEngine::Error,
         "verification of lowering to FIR failed");
     ci.getDiagnostics().Report(diagID);
     return false;

flang/lib/Optimizer/Transforms/CMakeLists.txt

add_flang_library(FIRTransforms
   StackArrays.cpp
   MemRefDataFlowOpt.cpp
   SimplifyRegionLite.cpp
   AlgebraicSimplification.cpp
   SimplifyIntrinsics.cpp
   AddDebugFoundation.cpp
   PolymorphicOpConversion.cpp
   LoopVersioning.cpp
+  OMPCaptureImplicitDeclTar.cpp
   
   DEPENDS
   FIRDialect
   FIROptTransformsPassIncGen
 
   LINK_LIBS
   FIRBuilder
   FIRCodeGen
   FIRDialect

flang/lib/Optimizer/Transforms/OMPCaptureImplicitDeclTar.cpp

+#include "flang/Optimizer/Dialect/FIRDialect.h"
+#include "flang/Optimizer/Dialect/FIROps.h"
+#include "flang/Optimizer/Dialect/FIRType.h"
+#include "flang/Optimizer/Transforms/Passes.h"
+#include "mlir/Dialect/Func/IR/FuncOps.h"
+#include "mlir/Dialect/LLVMIR/LLVMDialect.h"
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
+#include "mlir/IR/BuiltinDialect.h"
+#include "mlir/IR/Operation.h"
+#include "mlir/Pass/Pass.h"
+#include <mlir/IR/BuiltinOps.h>
+#include <mlir/IR/SymbolTable.h>
+#include <mlir/Support/LLVM.h>
+
+namespace fir {
+#define GEN_PASS_DEF_CAPTUREIMPLICITLYDECLARETARGETPASS
+#include "flang/Optimizer/Transforms/Passes.h.inc"
+} // namespace fir
+
+namespace {
+class CaptureImplicitlyDeclareTargetPass
+    : public fir::impl::CaptureImplicitlyDeclareTargetPassBase<
+          CaptureImplicitlyDeclareTargetPass> {
+
+  void markNestedFuncs(mlir::omp::DeclareTargetDeviceType parentDevTy,
+                       mlir::omp::DeclareTargetCaptureClause parentCapClause,
+                       mlir::Operation *currOp,
+                       std::vector<llvm::StringRef> visited,
+                       mlir::ModuleOp moduleOp) {
+    if (auto currFuncOp = llvm::dyn_cast<mlir::func::FuncOp>(currOp)) {
+      for (auto v : visited) {
+        if (currFuncOp.getSymName() == v)
+          return;
+      }
+      visited.push_back(currFuncOp.getSymName());
+    }
+
+    currOp->walk([&, this](fir::CallOp callOp) {
+      if (auto currFOp = moduleOp.lookupSymbol<mlir::func::FuncOp>(
+              callOp.getCallee().value())) {
+        auto current = llvm::dyn_cast<mlir::omp::DeclareTargetInterface>(
+            currFOp.getOperation());
+
+        if (current.isDeclareTarget()) {
+          auto currentDt = current.getDeclareTargetDeviceType();
+
+          // Found the same function twice, with different device_types, mark
+          // as Any as it belongs to both
+          if (currentDt != parentDevTy &&
+              currentDt != mlir::omp::DeclareTargetDeviceType::any) {
+            current.setDeclareTarget(mlir::omp::DeclareTargetDeviceType::any,
+                                     current.getDeclareTargetCaptureClause());
+          }
+        } else {
+          current.setDeclareTarget(parentDevTy, parentCapClause);
+        }
+
+        markNestedFuncs(parentDevTy, parentCapClause, currFOp, visited,
+                        moduleOp);
+      }
+    });
+  }
+
+  void runOnOperation() override {
+    mlir::ModuleOp moduleOp = getOperation();
+    for (auto functionOp : moduleOp.getOps<mlir::func::FuncOp>()) {
+      auto declareTargetOp = llvm::dyn_cast<mlir::omp::DeclareTargetInterface>(
+          functionOp.getOperation());
+      if (declareTargetOp.isDeclareTarget()) {
+        std::vector<llvm::StringRef> visited;
+        markNestedFuncs(declareTargetOp.getDeclareTargetDeviceType(),
+                        declareTargetOp.getDeclareTargetCaptureClause(),
+                        functionOp, visited, moduleOp);
+      }
+    }
+
+    moduleOp->walk([&](mlir::omp::TargetOp tarOp) {
+      std::vector<llvm::StringRef> visited;
+      markNestedFuncs(mlir::omp::DeclareTargetDeviceType::nohost,
+                      mlir::omp::DeclareTargetCaptureClause::to, tarOp, visited,
+                      moduleOp);
+    });
+  }
+};
+
+} // namespace
+
+namespace fir {
+std::unique_ptr<mlir::OperationPass<mlir::ModuleOp>>
+createCaptureImplicitlyDeclareTargetPass() {
+  return std::make_unique<CaptureImplicitlyDeclareTargetPass>();
+}
+} // namespace fir
 No newline at end of file

flang/test/Lower/OpenMP/omp-declare-target-implicit-func-and-subr-cap.f90

+!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED(TOGGLE) RESULT(K)
+    INTEGER :: I, J, K
+    LOGICAL :: TOGGLE
+    I = 10
+    J = 5
+    IF (TOGGLE) THEN
+        K = I
+    ELSE
+        K = J
+    END IF
+END FUNCTION IMPLICITLY_CAPTURED
+
+
+! CHECK-LABEL: func.func @_QPtarget_function
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+FUNCTION TARGET_FUNCTION(TOGGLE) RESULT(I)
+!$omp declare target
+    INTEGER :: I
+    LOGICAL :: TOGGLE
+    I = IMPLICITLY_CAPTURED(TOGGLE)
+END FUNCTION TARGET_FUNCTION
+
+!! -----
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_TWICE() RESULT(K)
+    INTEGER :: I
+    I = 10
+    K = I
+END FUNCTION IMPLICITLY_CAPTURED_TWICE
+
+! CHECK-LABEL: func.func @_QPtarget_function_twice_host
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (host), capture_clause = (to)>{{.*}}}
+FUNCTION TARGET_FUNCTION_TWICE_HOST() RESULT(I)
+!$omp declare target to(TARGET_FUNCTION_TWICE_HOST) device_type(host)
+    INTEGER :: I
+    I = IMPLICITLY_CAPTURED_TWICE()
+END FUNCTION TARGET_FUNCTION_TWICE_HOST
+
+! CHECK-LABEL: func.func @_QPtarget_function_twice_device
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+FUNCTION TARGET_FUNCTION_TWICE_DEVICE() RESULT(I)
+!$omp declare target to(TARGET_FUNCTION_TWICE_DEVICE) device_type(nohost)
+    INTEGER :: I
+    I = IMPLICITLY_CAPTURED_TWICE()
+END FUNCTION TARGET_FUNCTION_TWICE_DEVICE
+
+!! -----
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_nest
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_NEST() RESULT(K)
+    INTEGER :: I
+    I = 10
+    K = I
+END FUNCTION IMPLICITLY_CAPTURED_NEST
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_one
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to){{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_ONE() RESULT(K)
+    K = IMPLICITLY_CAPTURED_NEST()
+END FUNCTION IMPLICITLY_CAPTURED_ONE
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_two
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_TWO() RESULT(K)
+    INTEGER :: I
+    I = 10
+    K = I
+END FUNCTION IMPLICITLY_CAPTURED_TWO
+
+! CHECK-LABEL: func.func @_QPtarget_function_test
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+FUNCTION TARGET_FUNCTION_TEST() RESULT(J)
+!$omp declare target to(TARGET_FUNCTION_TEST) device_type(nohost)
+    INTEGER :: I, J
+    I = IMPLICITLY_CAPTURED_ONE()
+    J = IMPLICITLY_CAPTURED_TWO() + I
+END FUNCTION TARGET_FUNCTION_TEST
+
+!! -----
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_nest_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_NEST_TWICE() RESULT(K)
+    INTEGER :: I
+    I = 10
+    K = I
+END FUNCTION IMPLICITLY_CAPTURED_NEST_TWICE
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_one_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_ONE_TWICE() RESULT(K)
+    K = IMPLICITLY_CAPTURED_NEST_TWICE()
+END FUNCTION IMPLICITLY_CAPTURED_ONE_TWICE
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_two_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_TWO_TWICE() RESULT(K)
+    INTEGER :: I
+    I = 10
+    K = I
+END FUNCTION IMPLICITLY_CAPTURED_TWO_TWICE
+
+! CHECK-LABEL: func.func @_QPtarget_function_test_device
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+FUNCTION TARGET_FUNCTION_TEST_DEVICE() RESULT(J)
+    !$omp declare target to(TARGET_FUNCTION_TEST_DEVICE) device_type(nohost)
+        INTEGER :: I, J
+        I = IMPLICITLY_CAPTURED_ONE_TWICE()
+        J = IMPLICITLY_CAPTURED_TWO_TWICE() + I 
+END FUNCTION TARGET_FUNCTION_TEST_DEVICE
+
+! CHECK-LABEL: func.func @_QPtarget_function_test_host
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (host), capture_clause = (to)>{{.*}}}
+FUNCTION TARGET_FUNCTION_TEST_HOST() RESULT(J)
+    !$omp declare target to(TARGET_FUNCTION_TEST_HOST) device_type(host)
+        INTEGER :: I, J
+        I = IMPLICITLY_CAPTURED_ONE_TWICE()
+        J = IMPLICITLY_CAPTURED_TWO_TWICE() + I
+END FUNCTION TARGET_FUNCTION_TEST_HOST
+
+!! -----
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_with_dev_type_recursive
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+RECURSIVE FUNCTION IMPLICITLY_CAPTURED_WITH_DEV_TYPE_RECURSIVE(INCREMENT) RESULT(K)
+!$omp declare target to(IMPLICITLY_CAPTURED_WITH_DEV_TYPE_RECURSIVE) device_type(host)
+INTEGER :: INCREMENT, K
+IF (INCREMENT == 10) THEN
+    K = INCREMENT
+ELSE
+    K = IMPLICITLY_CAPTURED_WITH_DEV_TYPE_RECURSIVE(INCREMENT + 1)
+END IF
+END FUNCTION IMPLICITLY_CAPTURED_WITH_DEV_TYPE_RECURSIVE
+
+! CHECK-LABEL: func.func @_QPtarget_function_with_dev_type_recurse
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+FUNCTION TARGET_FUNCTION_WITH_DEV_TYPE_RECURSE() RESULT(I)
+!$omp declare target to(TARGET_FUNCTION_WITH_DEV_TYPE_RECURSE) device_type(nohost)
+INTEGER :: I
+I = IMPLICITLY_CAPTURED_WITH_DEV_TYPE_RECURSIVE(0)
+END FUNCTION TARGET_FUNCTION_WITH_DEV_TYPE_RECURSE
+
+!! -----
+
+MODULE test_module
+    CONTAINS
+! CHECK-LABEL: func.func @_QMtest_modulePimplicitly_captured_nest_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+    FUNCTION IMPLICITLY_CAPTURED_NEST_TWICE() RESULT(I)
+        INTEGER :: I
+        I = 10
+    END FUNCTION IMPLICITLY_CAPTURED_NEST_TWICE
+
+! CHECK-LABEL: func.func @_QMtest_modulePimplicitly_captured_one_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+    FUNCTION IMPLICITLY_CAPTURED_ONE_TWICE() RESULT(K)
+    !$omp declare target to(IMPLICITLY_CAPTURED_ONE_TWICE) device_type(host)
+        K = IMPLICITLY_CAPTURED_NEST_TWICE()
+    END FUNCTION IMPLICITLY_CAPTURED_ONE_TWICE
+    
+! CHECK-LABEL: func.func @_QMtest_modulePimplicitly_captured_two_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+    FUNCTION IMPLICITLY_CAPTURED_TWO_TWICE() RESULT(Y)
+        INTEGER :: Y
+        Y = 5
+    END FUNCTION IMPLICITLY_CAPTURED_TWO_TWICE
+    
+! CHECK-LABEL: func.func @_QMtest_modulePtarget_function_test_device
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+    FUNCTION TARGET_FUNCTION_TEST_DEVICE() RESULT(J)
+    !$omp declare target to(TARGET_FUNCTION_TEST_DEVICE) device_type(nohost)
+        INTEGER :: I, J
+        I = IMPLICITLY_CAPTURED_ONE_TWICE()
+        J = IMPLICITLY_CAPTURED_TWO_TWICE() + I 
+    END FUNCTION TARGET_FUNCTION_TEST_DEVICE
+END MODULE test_module
+
+!! -----
+
+PROGRAM MB
+    INTERFACE
+    SUBROUTINE CALLER_RECURSIVE
+        !$omp declare target to(CALLER_RECURSIVE) device_type(nohost)
+    END SUBROUTINE
+    
+    RECURSIVE SUBROUTINE IMPLICITLY_CAPTURED_RECURSIVE(INCREMENT)
+        INTEGER :: INCREMENT
+    END SUBROUTINE
+    END INTERFACE
+END PROGRAM
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_recursive
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+RECURSIVE SUBROUTINE IMPLICITLY_CAPTURED_RECURSIVE(INCREMENT)
+    INTEGER :: INCREMENT
+    IF (INCREMENT == 10) THEN
+        RETURN
+    ELSE
+        CALL IMPLICITLY_CAPTURED_RECURSIVE(INCREMENT + 1)
+    END IF
+END SUBROUTINE
+
+! CHECK-LABEL: func.func @_QPcaller_recursive
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+SUBROUTINE CALLER_RECURSIVE
+!$omp declare target to(CALLER_RECURSIVE) device_type(nohost)
+    CALL IMPLICITLY_CAPTURED_RECURSIVE(0)
+END SUBROUTINE

flang/test/Lower/OpenMP/omp-declare-target-implicit-tarop-cap.f90

+!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | FileCheck %s
+
+! CHECK-LABEL: func.func @_QPimplicit_capture
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICIT_CAPTURE() RESULT(I)
+    IMPLICIT NONE
+    INTEGER :: I  
+    I = 1
+END FUNCTION IMPLICIT_CAPTURE
+
+SUBROUTINE SUBR_TARGET()
+    INTEGER :: N
+!$omp target map(tofrom:N)
+    N = IMPLICIT_CAPTURE()
+!$omp end target
+END SUBROUTINE
+  
+!! -----
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_nest_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_NEST_TWICE() RESULT(I)
+    INTEGER :: I
+    I = 10
+END FUNCTION IMPLICITLY_CAPTURED_NEST_TWICE
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_one_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (any), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_ONE_TWICE() RESULT(K)
+!$omp declare target to(IMPLICITLY_CAPTURED_ONE_TWICE) device_type(host)
+    K = IMPLICITLY_CAPTURED_NEST_TWICE()
+END FUNCTION IMPLICITLY_CAPTURED_ONE_TWICE
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_two_twice
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+FUNCTION IMPLICITLY_CAPTURED_TWO_TWICE() RESULT(Y)
+    INTEGER :: Y
+    Y = 5
+END FUNCTION IMPLICITLY_CAPTURED_TWO_TWICE
+
+
+FUNCTION TARGET_FUNCTION_TEST_DEVICE() RESULT(J)
+    INTEGER :: I, J
+    !$omp target map(tofrom: I, J)
+    I = IMPLICITLY_CAPTURED_ONE_TWICE()
+    J = IMPLICITLY_CAPTURED_TWO_TWICE() + I 
+    !$omp end target
+END FUNCTION TARGET_FUNCTION_TEST_DEVICE
+
+!! -----
+
+! CHECK-LABEL: func.func @_QPimplicitly_captured_recursive
+! CHECK-SAME: {{.*}}attributes {omp.declare_target = #omp.declaretarget<device_type = (nohost), capture_clause = (to)>{{.*}}}
+RECURSIVE FUNCTION IMPLICITLY_CAPTURED_RECURSIVE(INCREMENT) RESULT(K)
+    INTEGER :: INCREMENT, K
+    IF (INCREMENT == 10) THEN
+        K = INCREMENT
+    ELSE
+        K = IMPLICITLY_CAPTURED_RECURSIVE(INCREMENT + 1)
+    END IF
+END FUNCTION IMPLICITLY_CAPTURED_RECURSIVE
+
+FUNCTION TARGET_FUNCTION_RECURSE() RESULT(I)
+    INTEGER :: I
+    !$omp target map(tofrom: I)
+    I = IMPLICITLY_CAPTURED_RECURSIVE(0)
+    !$omp end target
+END FUNCTION TARGET_FUNCTION_RECURSE
 No newline at end of file