This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP] A semantic analysis pass for marking functions, subroutines and their interfaces as implicitly declare target when called inside of a declare target function or target region
AbandonedPublic

Authored by agozillon on May 10 2023, 4:03 PM.

Details

Summary

This patch adds a new series of passes (currently only 1 and two
helper pass) called FinalizeOMP which are to be executed as
the final OpenMP pass step before semantic error checking, which
allows previous passes to have finished their rewriting
and symbol resolution giving the passes all the information
they need to work.

The main pass in FinalizeOMP currently is ImplicitDeclareTargetCapture
which scours any declare target (nohost|any) function or target region for implicit
function calls (and the children of these invocations calls) which currently
are not marked as declare target and then marks them as such by modifying
the existing declare target or generating a new declare target with the any
device_type.

Diff Detail

Event Timeline

agozillon created this revision.May 10 2023, 4:03 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
agozillon requested review of this revision.May 10 2023, 4:03 PM
agozillon updated this revision to Diff 521134.May 10 2023, 4:07 PM

Missing newline at end of test file...

agozillon updated this revision to Diff 521138.May 10 2023, 4:12 PM
  • update comment to reflect the slight changes to the pass

This is the semantic pass segment of the declare target patch: https://reviews.llvm.org/D146063 However, this adds a new test for modules (which is just a replication of the other test inside of a module to make sure it functions as it should) and added documentation on the pass as requested. The pass works *slightly* different to the original one in D146063, rather than storing a ProgramUnit it holds a std::variant of SubroutineSubprogram's and FuncitonSubprograms allowing the pass to be a little more generally applicable (Thank you @kiranchandramohan for pointing out this flaw/requesting for the test).

agozillon added a comment.EditedMay 11 2023, 5:45 AM

Appears to be one buildbot failure caused by the new changes I made to this segment, for the examples that I didn't have active on my machine while testing, looking into it and i'll hopefully have a fix soon. Which may just be an small alteration to the test to have it suit the new semantic pass.

agozillon updated this revision to Diff 521284.May 11 2023, 6:13 AM
  • update comment to reflect the slight changes to the pass
  • Amend test that was broken as side affect of new pass
agozillon updated this revision to Diff 521285.May 11 2023, 6:14 AM
  • Amend test that was broken as side affect of new pass

Small tweak to the omp-declarative-directive.f90 test, which should fix it's failure in the buildbot due to the changes the pass introduces.

Declare target is allowed inside interface declarations. This seems to run into some issue here. Could you check the following two tests?

program mb
  interface
    subroutine foo
      !$omp declare target
    end subroutine
  end interface
end program

subroutine foo
  print *, "foo"
end subroutine
module m
contains
 subroutine s1(i)
   integer :: i
   print *, i
 end subroutine
 subroutine s2(r)
   real :: r
   print *, r
 end subroutine
end

subroutine s
use m
!$omp declare target to(s)
interface ss
module procedure s1, s2
end interface
integer :: x = 1
real :: y = 2.2
call ss(x)
call ss(y)
end subroutine

Also, the following

subroutine sub                                           
  implicit none                                             
  !$omp declare target to(sub)
  integer :: n

  n = forced()
contains

function forced()
 implicit none
 integer:: forced

 forced = 1
end function forced

end subroutine

Thank you very much, I'm more than happy to, I am trying to update another patch at the moment, but I'll get around to these ASAP. If there's any other tests or examples please don't hesitate to add them to the list! My Fortran isn't all that great yet, so there's certain things I will accidentally miss testing due to lack of knowledge unfortunately, I had a skim over the OpenMP specifications repository examples for declare target but didn't find anything as complex, they all seemed rather simple, although I haven't been looking for anything complex just yet! But these are very nice additions to look into, thank you.

There will likely be a further update to this pass at some point in the near future to support marking functions in target regions as implicitly declare target, as this is stated in the newest iteration of the specification, I've started on it and made some progress, but it will likely be a follow up patch as I don't want to overcomplicate this initial patch anymore than it needs to be!

agozillon updated this revision to Diff 525181.May 24 2023, 7:44 AM
  • Rebase and overhaul and extension of pass and additional tests
agozillon retitled this revision from [Flang][OpenMP][MLIR] Add initial semantic analysis pass for marking functions and subroutines as implicitly declare target to [Flang][OpenMP] A semantic analysis pass for marking functions, subroutines and their interfaces as implicitly declare target when called inside of a declare target function or target region .May 24 2023, 7:48 AM
agozillon edited the summary of this revision. (Show Details)

The patch has went a chunky overhaul, I've added several new test sets including the ones brought up by @kiranchandramohan, thank you again. The behavior of the pass has changed, it now supports implicit marking of functions called in target regions as well as declare target functions, and it now generates declare targets rather than modifying the originating declare target, this is in part to support target region which has no declare target, but also as it is perhaps more correct behavior. The pass also needed some tweaking to support the prior test cases brought up in the review, e.g. handling of interfaces, the pass will now update function interfaces when their respective implementation is altered, it will not clone the implementations declare target to the interface, instead it simply tries to alter it using the same information, if the interface and functions declare target differ, then the end result will also differ, this is so that we are not rewriting user code to be a false-positive, OpenMP demands that the declare target in the interface and implementation match, so if they write them incorrectly, we should not try to correct their mistake. Although perhaps we should check the declare target and interface are equivalent before we do any alterations, although, it is harmless in either case, it should error out in the later semantic pass, it will depend how the diagnostics look when the error test/check is implemented. But yes, please feel free to discuss the behavior or ask questions or give new test cases and examples.

If anyone has any time, a review on this would be greatly appreciated, although the lowering in https://reviews.llvm.org/D150329 should take precedence if everyone is short on time (I am aware this is unfortunately a rather large patch), as this is an addition to improve the declare target support rather than something that is entirely necessary for it to function.

If it would be possible to have an initial review of this patch whenever anyone has any time I would greatly appreciate it, I do understand that it's a little bit of a monolith, so I do apologies for that. The majority is tests though, I promise! And that's to make sure things are tested to a reasonable extent, although I am sure there's edge cases that are perhaps unconsidered in the tests, so please do point any out you spot/can think of, especially relating to Fortran caveats I may be unaware of. Thank you very much ahead of time.

jsjodin added inline comments.Jun 13 2023, 9:13 AM
flang/lib/Semantics/finalize-omp.cpp
157

No need to use a lambda function.

178

A more descriptive variable name would help.

243

No need for control flow here, just return.

418

Optional braces, maybe add to all of them or remove from all.

agozillon marked 3 inline comments as done.
agozillon retitled this revision from [Flang][OpenMP] A semantic analysis pass for marking functions, subroutines and their interfaces as implicitly declare target when called inside of a declare target function or target region to [Flang][OpenMP] A semantic analysis pass for marking functions, subroutines and their interfaces as implicitly declare target when called inside of a declare target function or target region.
  • Rebase and overhaul and extension of pass and additional tests
  • Add minor cleanup as requested by reviewer
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

Recent commit rebased and tried to address the previous reviewer comments!

If it would be possible to get a further review on this it would be much appreciated.

agozillon abandoned this revision.Jul 17 2023, 5:20 AM

I am going to close this for now as the alternative method; utilising a transformation pass, has been accepted. This patch can be revisited if we ever need it.