Page MenuHomePhabricator

[flang] [openmp] Add Fortran specific semantic check 4 for OpenMP Allocate directive.
ClosedPublic

Authored by I_Perry on May 13 2021, 6:41 AM.

Details

Summary

This patch adds the 4th Fortran specific semantic check for the OpenMP
allocate directive: "If a list item has the SAVE attribute, is a common
block name, or is declared in the scope of a module, then only predefined
memory allocator parameters can be used in the allocator clause".

Code in this patch was based on code from https://reviews.llvm.org/D93549/new/.

Diff Detail

Unit TestsFailed

TimeTest
80 msx64 debian > Flang.Semantics::omp-allocate08.f90
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/flang/test/Semantics/test_errors.sh /var/lib/buildkite-agent/builds/llvm-project/flang/test/Semantics/omp-allocate08.f90 /var/lib/buildkite-agent/builds/llvm-project/build/tools/flang/test/Semantics/Output/omp-allocate08.f90.tmp /var/lib/buildkite-agent/builds/llvm-project/build/bin/flang-new -fc1 -fopenmp
140 msx64 windows > LLVM.Other::new-pm-lto-defaults.ll
Script: -- : 'RUN: at line 5'; c:\ws\w3\llvm-project\premerge-checks\build\bin\opt.exe -disable-verify -verify-cfg-preserved=0 -debug-pass-manager -passes='lto<O1>' -S C:\ws\w3\llvm-project\premerge-checks\llvm\test\Other\new-pm-lto-defaults.ll 2>&1 | c:\ws\w3\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w3\llvm-project\premerge-checks\llvm\test\Other\new-pm-lto-defaults.ll --check-prefix=CHECK-O --check-prefix=CHECK-O1

Event Timeline

I_Perry created this revision.May 13 2021, 6:41 AM
I_Perry requested review of this revision.May 13 2021, 6:41 AM
Herald added a project: Restricted Project. · View Herald Transcript
flang/test/Semantics/omp-allocate08.f90
33

Can you add a test with a custom allocator?

I_Perry updated this revision to Diff 346145.May 18 2021, 5:10 AM

Added custom allocator test in omp-allocate08.f90

Thanks @I_Perry for the changes. Do you know why the allocate08 test fails?

flang/lib/Semantics/check-omp-structure.cpp
659

We should not hardcode this number 8.

I_Perry updated this revision to Diff 347363.May 24 2021, 6:00 AM

Made change to fix failing test on Phabricator.

I_Perry marked an inline comment as done.May 25 2021, 5:13 AM
I_Perry updated this revision to Diff 349018.Jun 1 2021, 10:37 AM

Added function to check for predefined allocator and allocator value generated enum.

kiranchandramohan requested changes to this revision.Jun 3 2021, 6:15 AM

The test omp-allocate08.f90 is failing, please check.

flang/lib/Semantics/check-omp-structure.cpp
663

For a predefined allocator, this obtains the integer value corresponding to the allocator. But for a custom allocator a string is obtained now. I believe the GetIntValue can be any random value based on the name of the string. We need to do something different here. (like one of the following)

  1. Check whether the Expression contains an int or string before calling GetIntValue.
  2. Change the parsing to place the custom allocator's value in Expr
Predefined Allocator
| | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr = '1_4'
Custom Allocator
| | | OmpClauseList -> OmpClause -> Allocator -> Scalar -> Integer -> Expr = 'custom_allocator'
This revision now requires changes to proceed.Jun 3 2021, 6:15 AM
llvm/include/llvm/Frontend/OpenMP/OMP.td
43

The following is the list of allocators found from openmp/runtime/src/kmp_global.cpp.

  1. There is no omp_custom_allocator.
  2. There are a few allocators with value above 100.
omp_allocator_handle_t const omp_null_allocator = NULL;
omp_allocator_handle_t const omp_default_mem_alloc =
    (omp_allocator_handle_t const)1;
omp_allocator_handle_t const omp_large_cap_mem_alloc =
    (omp_allocator_handle_t const)2;
omp_allocator_handle_t const omp_const_mem_alloc =
    (omp_allocator_handle_t const)3;
omp_allocator_handle_t const omp_high_bw_mem_alloc =
    (omp_allocator_handle_t const)4;
omp_allocator_handle_t const omp_low_lat_mem_alloc =
    (omp_allocator_handle_t const)5;
omp_allocator_handle_t const omp_cgroup_mem_alloc =
    (omp_allocator_handle_t const)6;
omp_allocator_handle_t const omp_pteam_mem_alloc =
    (omp_allocator_handle_t const)7;
omp_allocator_handle_t const omp_thread_mem_alloc =
    (omp_allocator_handle_t const)8;
// Preview of target memory support
omp_allocator_handle_t const llvm_omp_target_host_mem_alloc =
    (omp_allocator_handle_t const)100;
omp_allocator_handle_t const llvm_omp_target_shared_mem_alloc =
    (omp_allocator_handle_t const)101;
omp_allocator_handle_t const llvm_omp_target_device_mem_alloc =
    (omp_allocator_handle_t const)102;
omp_allocator_handle_t const kmp_max_mem_alloc =
    (omp_allocator_handle_t const)1024;
I_Perry updated this revision to Diff 349704.Jun 3 2021, 3:06 PM

Removed generated enum as a simpler test for a custom allocator was found.

kiranchandramohan requested changes to this revision.Jun 4 2021, 6:17 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
259

Nit: Can this be limited to 80 characters per line?

265

What about here? I think common blocks come here? Can you add a test and check?

658

Nit: Add a comment explaining why this code works in identifying predefined allocators.

flang/lib/Semantics/check-omp-structure.h
235

Nit:Can this be initialized to a value?

This revision now requires changes to proceed.Jun 4 2021, 6:17 AM
I_Perry updated this revision to Diff 350875.Jun 9 2021, 6:08 AM

Addressed nit comments and added additional test for common block names.

This revision is now accepted and ready to land.Jun 10 2021, 12:35 AM
This revision was landed with ongoing or failed builds.Jun 15 2021, 8:04 AM
This revision was automatically updated to reflect the committed changes.