This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP][MLIR] Add lowering from PFT to MLIR (FIR) for OpenMP declare target directive in Flang
ClosedPublic

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

Details

Summary

This patch adds PFT lowering for the OpenMP declare target directive
in Flang to the omp dialects declare target attribute, which currently
applies to function or global operations.

This builds on subsequent patches which add a semantic analysis pass
for implicit declare target capture and another which adds the declare
target MLIR attribute itself. Further lowering of the MLIR attribute comes
in additional patches on top of this.

Diff Detail

Event Timeline

agozillon created this revision.May 10 2023, 4:31 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
agozillon requested review of this revision.May 10 2023, 4:31 PM
agozillon added a comment.EditedMay 10 2023, 4:38 PM

This is the PFT lowering to MLIR segment of the larger declare target patch: https://reviews.llvm.org/D146063 This segment has underwent no additions or changes during the split. Although, it will currently fail the buildbot as this patch isn't built on top of the other patches using arcanist, that's some arcanist wizardry I still need to learn.

agozillon added a comment.EditedMay 11 2023, 5:42 AM
This comment has been deleted.

Wrong patch! Was intended for the semantic analysis segment... so I've deleted the misinformation

I believe this patch can go in without the semantic analysis pass patch (but it depends on the attribute patch), I just might need to remove some tests that depend on the pass and move them to the semantic pass patch for later addition, so if it would be possible to get a seperate review on this it would be appreciated.

As the attribute has currently been accepted it would be great to get a review on this segment which lowers from the PFT to the attribute!

Could you fix the CI issues?

Could you fix the CI issues?

No problem at all!

agozillon updated this revision to Diff 526599.May 30 2023, 7:03 AM
  • [Flang][OpenMP][MLIR] Rebase and remove tests dependent on other components

latest patch should hopefully appease the CI, it involved a rebase and removal of some tests that will fail without the semantic analysis pass (and are now already tested in that patch).

Looks OK.

In general, can you spell the types unless the type is on the RHS?

Variables in the main program have an implicit save and can hence be treated as globals. The following code will currently crash. I believe we have the same issue with threadprivate as well. If you can catch this and issue an appropriate TODO error that would be great.

program main
  integer :: i
  !$omp declare target to(i)
end
flang/lib/Lower/OpenMP.cpp
2518

Can you add a Fortran common block example to test this?

2565
2566

Can you add an assert here?

2572
agozillon marked 3 inline comments as done.Jun 1 2023, 8:54 AM

Looks OK.

In general, can you spell the types unless the type is on the RHS?

Variables in the main program have an implicit save and can hence be treated as globals. The following code will currently crash. I believe we have the same issue with threadprivate as well. If you can catch this and issue an appropriate TODO error that would be great.

program main
  integer :: i
  !$omp declare target to(i)
end

Working on these alterations at the moment, it might take a little longer than I initially expected as it seems common blocks aren't accepted in declare target clauses at the moment, at least the following segment doesn't work at the moment:

PROGRAM main
    REAL :: one = 1
    REAL :: two = 2
    COMMON /numbers/ one, two
    !$omp declare target(/numbers/)
END

It results in a semantic error at the moment. I believe the above segment of code is the correct way to provide a common block to a clause (specifying just "numbers" works, but it results in the incorrect symbol and I do not think that's the correct way to specify the name from examples I've seen and a little test with gfortran), but please do point out if it is incorrect, I'm rather unfamiliar with them.

So I'll have a look at how easy that is to fix, if it is the correct syntax.

Looks OK.

In general, can you spell the types unless the type is on the RHS?

Variables in the main program have an implicit save and can hence be treated as globals. The following code will currently crash. I believe we have the same issue with threadprivate as well. If you can catch this and issue an appropriate TODO error that would be great.

program main
  integer :: i
  !$omp declare target to(i)
end

Working on these alterations at the moment, it might take a little longer than I initially expected as it seems common blocks aren't accepted in declare target clauses at the moment, at least the following segment doesn't work at the moment:

PROGRAM main
    REAL :: one = 1
    REAL :: two = 2
    COMMON /numbers/ one, two
    !$omp declare target(/numbers/)
END

It results in a semantic error at the moment. I believe the above segment of code is the correct way to provide a common block to a clause (specifying just "numbers" works, but it results in the incorrect symbol and I do not think that's the correct way to specify the name from examples I've seen and a little test with gfortran), but please do point out if it is incorrect, I'm rather unfamiliar with them.

So I'll have a look at how easy that is to fix, if it is the correct syntax.

That seems to be right. We have similar tests for threadprivate (https://github.com/llvm/llvm-project/blob/main/flang/test/Lower/OpenMP/threadprivate-commonblock.f90). You can check whether there is any special handling for threadprivate.

Added declare target to the resolution pass in the following patch: https://reviews.llvm.org/D151993 this resolves the symbol for common block (and any other symbols that may need similarly resolved) when captured by the directive, I'll add common block tests in this patch as suggested and it'll test the resolution works appropriately by side effect.

agozillon updated this revision to Diff 527942.Jun 2 2023, 1:17 PM
agozillon marked an inline comment as done.
  • [Flang][OpenMP][MLIR] Rebase and remove tests dependent on other components
  • [Flang][OpenMP][MLIR] Apply reviewer feedback where possible
agozillon updated this revision to Diff 527943.Jun 2 2023, 1:21 PM
  • [Flang][OpenMP][Test] Add missing space
agozillon added a comment.EditedJun 2 2023, 1:30 PM

Added the common block tests and applied the feedback you mentioned! Please have a look incase you spot something I missed from your feedback.

I haven't added code to catch the case you mentioned, e.g.

program main
  integer :: i
  !$omp declare target to(i)
end

As it's currently caught by the assert that's been added and I'm not too sure how to catch this case from inside of the lowering (so I've added some inline comments above the assert for the known cases it'll trigger).

However, threadprivate currently handles this case, it seems to do so by generating an internal fir.GlobalOp which has the symbol information, allowing it to be searched (unlike the fir.AllocaOp) amongst other things. So, it could be possible to do something similar for declare target, upgrade the fir.AllocaOp to include a fir.GlobalOp, alternatively, a search method for the bind or unique name could be made that functions similarly to findSymbol. In either case, that seems like it extends into a future follow up patch.

kiranchandramohan accepted this revision.Jun 2 2023, 2:13 PM

LG. It will be great if someone in your team. can have a look too.

flang/lib/Lower/OpenMP.cpp
2580–2581

Convert this to a TODO. After fixing the Case 3, we can convert it to an assert. This is to avoid adding crashes from gfortran testsuite.

This revision is now accepted and ready to land.Jun 2 2023, 2:13 PM
agozillon updated this revision to Diff 527990.Jun 2 2023, 2:39 PM
  • [Flang][OpenMP] Replace assert with TODO
agozillon marked an inline comment as done.Jun 2 2023, 2:41 PM

LG. It will be great if someone in your team. can have a look too.

No problem, I'll see who's available on my team and have them give a final review the patch, thank you very much for your help and time @kiranchandramohan

Opened this patch related to the TODO discussed within this patch: https://reviews.llvm.org/D152037 feel free to review and discuss it there for those interested.

No more spam from me tonight, sorry everyone that's subjected to my patches!

skatrak accepted this revision.Jun 5 2023, 4:24 AM

Small nit, otherwise LGTM.

flang/lib/Lower/OpenMP.cpp
2529

Nit: I think in this file it's preferred object initialization using "=" rather than braces. So, to keep the same coding style, I'd recommend changing the initializations of spec, objectList, clauseList, toClause, linkClause and deviceClause below.

Thank you both for the reviews!

flang/lib/Lower/OpenMP.cpp
2529

it seems fairly normal for unwrap to be used this way, elsewhere in the file as well, the get/get_if is a little different though. So I'll change those instances on commit but not the unwraps to keep the status quo

flang/lib/Lower/OpenMP.cpp