[openmp][flang] Add tests for map clause

[shraiysh]

This patch adds basic tests for map clause on target construct for commonblocks. There will be more tests to add, which will be added in future patches. Currently failing tests are added in a separate folder with XFAIL. They should be moved as they are fixed.

[shiltian]

LG

[agozillon]

awesome to see this working! :D admittedly have never tried it

[agozillon]

Awesome work, it would be cool to have an example like the following eventually:

program main
    use omp_lib
    integer :: var1, var2
    common /var_common/ var1, var2
    var1 = 10
    var2 = 20
    print *, "var4 before target = ", var1
    !$omp target map(tofrom:var_common)
      var1 = var2
    !$omp end target
    print *, "var4 after target = ", var1

    if (var1 /= 20) then
        print*, "======= FORTRAN Test Failed! ======="
        stop 1    
    end if
  end program

Which I believe is legal Fortran+OpenMP @mjklemm or @kiranchandramohan may be able to clarify completely. The above example doesn't appear to work upstream or downstream from what I can tell though, so likely something for a future PR where we add the features needed to run it. The proper syntax for the map (at least from looking at some examples) also doesn't seem to get accepted currently either, I think it should be possible to write:

!$omp target map(tofrom:/var_common/)

With the"/" slashes, but the symbol unfortunately doesn't appear to be found currently when utilised like that. Someone with more Fortran and OpenMP knowledge might know better though, I am very unfamiliar with common blocks.

But yes, things for the future. Otherwise this PR LGTM, nice work.

[TIFitis]

Looks good 👍🏽

[mjklemm]

The correct spelling would be !$omp target map(tofrom:/var_common/), see OpenMP 5.2 specification page 61, line 6.

[agozillon]

The correct spelling would be !$omp target map(tofrom:/var_common/), see OpenMP 5.2 specification page 61, line 6.

Thank you @mjklemm ! Weirdly the /var_common/ is not accepted but the compiler is happy with var_common currently, although it doesn't appear to do anything reasonable with it. So it looks like the map clause needs to be taught how to understand the correct spelling.

[kiranchandramohan]

For threadprivate, that syntax is accepted.

llvm-project/flang/test/Lower/OpenMP/FIR/threadprivate-commonblock.f90

Line 13 in deb429e

!$omp threadprivate(/blk/)

[agozillon]

Seems to work for declare target link (and probably to as well). Just seems to be map that I get the following error for:

error: Internal: no symbol found for 'var_common'

When providing the correct syntax.

[mjklemm]

The correct spelling would be !$omp target map(tofrom:/var_common/), see OpenMP 5.2 specification page 61, line 6.

Thank you @mjklemm ! Weirdly the /var_common/ is not accepted but the compiler is happy with var_common currently, although it doesn't appear to do anything reasonable with it. So it looks like the map clause needs to be taught how to understand the correct spelling.

I'm smelling a bug here. :-)

[shraiysh]

Thanks for the reviews! I will add more cases, that seem to be working and failing. Should I file issues for failing cases or is it okay to just add them as failing testcases?

[agozillon]

Thanks for the reviews! I will add more cases, that seem to be working and failing. Should I file issues for failing cases or is it okay to just add them as failing testcases?

Unfortunately not sure if there is a place for failing test cases in upstream llvm (someone else likely knows better than me though).

However, opening issues may be the best way to track them and then downstream we also have: https://github.com/ROCm-Developer-Tools/aomp/tree/aomp-dev/test/smoke-fort-fails which you could place the test cases in for the interim (and perhaps add the opened issue number it correlates to). These tests are ran nightly from what I understand with both our downstream branch and upstream llvm, once they start passing, they can be moved to the pass folder + upstreamed (if they haven't been already).

[shraiysh]

As a part of this patch, I have added two failing tests - openmp/libomptarget/test/offloading/fortran/failing/target_map_common_block1.f90 and openmp/libomptarget/test/offloading/fortran/failing/target_map_common_block2.f90. I have put XFAIL on that. I don't remember in which subproject, but I saw a similar pattern for failing tests in some other subdirectory. When it gets implemented, we can move that test outside the failing directory. I was just wondering if I should open issues for those or not.

[agozillon]

Ah both these cases work downstream which is what I tested with then glossed over the xfail and failing directory, that's my bad.

I'd lean towards open up issues on things and point towards the file that's failing when it's added as a test to the failing directory (and perhaps add the issue number or link as a comment to the failing test, so people can close the issue once it's fixed, i.e. when they see the test fail locally after making changes that fix it and move the test).

Although, for both of these failing cases I'm not sure it's worth generating an issue, the second with implicit capture should work after @TIFitis IsolatedFromAbove patch series and the first should work with the changes added in the array sectioning patch! However, feel free to open up an issue on them if you wish!

[shraiysh]

Okay, I will open the issues and assign them to @TIFitis and you to keep track of it. Thanks for the details.

[jdoerfert]

FWIW, this will sooner or later blow up. You assume there is only one GPU. I'd recommend to do something like:

print omp_get_num_devices()
!CHECK: [[ND:[0-9]*]]
print omp_get_default_device()
!CHECK: [[DD:[0-9]*]]
!CHECK: devices: [[ND]] [[DD]]

[shraiysh]

Thanks for pointing this out.

If possible, I'd like to do something like this, and make sure this runs only if there is a device. Is it possible to do this with LIT directives to ensure this test runs when there is atleast one device, and doesn't run (unsupported) otherwise?

print omp_get_num_devices()
!CHECK: [[ND:[1-9]*]]