[clang][OpenMP] Rewrite `getOpenMPCaptureRegions` in terms of leafs #97110

kparzysz · 2024-06-28T20:40:02Z

Replace the switch in getOpenMPCaptureRegions with a loop collecting capture regions based on the constituent directives.

What is considered "executable" in clang differs slightly from the OpenMP's "executable" category. In addition to the executable category, subsidiary directives, and OMPD_error are considered executable. Implement a function that performs that check.

Check if the given directive can capture variables, and thus needs a captured statement. Simplify some code using this function.

…/c02-is-capturing

Replace the switch in `getOpenMPCaptureRegions` with a loop collecting capture regions based on the constituent directives.

llvmbot · 2024-06-28T20:40:32Z

@llvm/pr-subscribers-clang

Author: Krzysztof Parzyszek (kparzysz)

Changes

Replace the switch in getOpenMPCaptureRegions with a loop collecting capture regions based on the constituent directives.

Full diff: https://github.com/llvm/llvm-project/pull/97110.diff

1 Files Affected:

(modified) clang/lib/Basic/OpenMPKinds.cpp (+72-98)

diff --git a/clang/lib/Basic/OpenMPKinds.cpp b/clang/lib/Basic/OpenMPKinds.cpp
index 30c34c207ae23..152891dfa27dc 100644
--- a/clang/lib/Basic/OpenMPKinds.cpp
+++ b/clang/lib/Basic/OpenMPKinds.cpp
@@ -747,105 +747,79 @@ void clang::getOpenMPCaptureRegions(
   assert(unsigned(DKind) < llvm::omp::Directive_enumSize);
   assert(isOpenMPCapturingDirective(DKind));
 
-  switch (DKind) {
-  case OMPD_metadirective:
-    CaptureRegions.push_back(OMPD_metadirective);
-    break;
-  case OMPD_parallel:
-  case OMPD_parallel_for:
-  case OMPD_parallel_for_simd:
-  case OMPD_parallel_master:
-  case OMPD_parallel_masked:
-  case OMPD_parallel_sections:
-  case OMPD_distribute_parallel_for:
-  case OMPD_distribute_parallel_for_simd:
-  case OMPD_parallel_loop:
-    CaptureRegions.push_back(OMPD_parallel);
-    break;
-  case OMPD_target_teams:
-  case OMPD_target_teams_distribute:
-  case OMPD_target_teams_distribute_simd:
-    CaptureRegions.push_back(OMPD_task);
-    CaptureRegions.push_back(OMPD_target);
-    CaptureRegions.push_back(OMPD_teams);
-    break;
-  case OMPD_teams:
-  case OMPD_teams_distribute:
-  case OMPD_teams_distribute_simd:
-    CaptureRegions.push_back(OMPD_teams);
-    break;
-  case OMPD_target:
-  case OMPD_target_simd:
-    CaptureRegions.push_back(OMPD_task);
-    CaptureRegions.push_back(OMPD_target);
-    break;
-  case OMPD_teams_loop:
-  case OMPD_teams_distribute_parallel_for:
-  case OMPD_teams_distribute_parallel_for_simd:
-    CaptureRegions.push_back(OMPD_teams);
-    CaptureRegions.push_back(OMPD_parallel);
-    break;
-  case OMPD_target_parallel:
-  case OMPD_target_parallel_for:
-  case OMPD_target_parallel_for_simd:
-  case OMPD_target_parallel_loop:
-    CaptureRegions.push_back(OMPD_task);
-    CaptureRegions.push_back(OMPD_target);
-    CaptureRegions.push_back(OMPD_parallel);
-    break;
-  case OMPD_task:
-  case OMPD_target_enter_data:
-  case OMPD_target_exit_data:
-  case OMPD_target_update:
-    CaptureRegions.push_back(OMPD_task);
-    break;
-  case OMPD_taskloop:
-  case OMPD_taskloop_simd:
-  case OMPD_master_taskloop:
-  case OMPD_master_taskloop_simd:
-  case OMPD_masked_taskloop:
-  case OMPD_masked_taskloop_simd:
-    CaptureRegions.push_back(OMPD_taskloop);
-    break;
-  case OMPD_parallel_masked_taskloop:
-  case OMPD_parallel_masked_taskloop_simd:
-  case OMPD_parallel_master_taskloop:
-  case OMPD_parallel_master_taskloop_simd:
-    CaptureRegions.push_back(OMPD_parallel);
-    CaptureRegions.push_back(OMPD_taskloop);
-    break;
-  case OMPD_target_teams_loop:
-  case OMPD_target_teams_distribute_parallel_for:
-  case OMPD_target_teams_distribute_parallel_for_simd:
-    CaptureRegions.push_back(OMPD_task);
-    CaptureRegions.push_back(OMPD_target);
-    CaptureRegions.push_back(OMPD_teams);
-    CaptureRegions.push_back(OMPD_parallel);
-    break;
-  case OMPD_nothing:
-    CaptureRegions.push_back(OMPD_nothing);
-    break;
-  case OMPD_loop:
-    // TODO: 'loop' may require different capture regions depending on the bind
-    // clause or the parent directive when there is no bind clause. Use
-    // OMPD_unknown for now.
-  case OMPD_simd:
-  case OMPD_for:
-  case OMPD_for_simd:
-  case OMPD_sections:
-  case OMPD_single:
-  case OMPD_taskgroup:
-  case OMPD_distribute:
-  case OMPD_ordered:
-  case OMPD_target_data:
-  case OMPD_distribute_simd:
-  case OMPD_scope:
-  case OMPD_dispatch:
+  auto getRegionsForLeaf = [&](OpenMPDirectiveKind LKind) {
+    assert(isLeafConstruct(LKind) && "Epecting leaf directive");
+    switch (LKind) {
+    case OMPD_metadirective:
+      CaptureRegions.push_back(OMPD_metadirective);
+      break;
+    case OMPD_nothing:
+      CaptureRegions.push_back(OMPD_nothing);
+      break;
+    case OMPD_parallel:
+      CaptureRegions.push_back(OMPD_parallel);
+      break;
+    case OMPD_target:
+      CaptureRegions.push_back(OMPD_task);
+      CaptureRegions.push_back(OMPD_target);
+      break;
+    case OMPD_task:
+    case OMPD_target_enter_data:
+    case OMPD_target_exit_data:
+    case OMPD_target_update:
+      CaptureRegions.push_back(OMPD_task);
+      break;
+    case OMPD_teams:
+      CaptureRegions.push_back(OMPD_teams);
+      break;
+    case OMPD_taskloop:
+      CaptureRegions.push_back(OMPD_taskloop);
+      break;
+    case OMPD_loop:
+      // TODO: 'loop' may require different capture regions depending on the
+      // bind clause or the parent directive when there is no bind clause.
+      // If any of the directives that push regions here are parents of 'loop',
+      // assume 'parallel'. Otherwise do nothing.
+      if (!CaptureRegions.empty() &&
+          !llvm::is_contained(CaptureRegions, OMPD_parallel))
+        CaptureRegions.push_back(OMPD_parallel);
+      break;
+    case OMPD_dispatch:
+    case OMPD_distribute:
+    case OMPD_for:
+    case OMPD_masked:
+    case OMPD_master:
+    case OMPD_ordered:
+    case OMPD_scope:
+    case OMPD_sections:
+    case OMPD_simd:
+    case OMPD_single:
+    case OMPD_target_data:
+    case OMPD_taskgroup:
+      // These directives (when standalone) use OMPD_unknown as the region,
+      // but when they're constituents of a compound directive, and other
+      // leafs from that directive have specific regions, then these directives
+      // add no additional regions.
+      break;
+    default:
+      llvm::errs() << getOpenMPDirectiveName(LKind) << '\n';
+      llvm_unreachable("Unexpected directive");
+    }
+  };
+
+  for (OpenMPDirectiveKind L : getLeafConstructsOrSelf(DKind))
+    getRegionsForLeaf(L);
+
+  // If no regions were added, then only the leafs using "unknown" were
+  // present. Push a single OMPD_unknown as the capture region.
+  if (CaptureRegions.empty())
     CaptureRegions.push_back(OMPD_unknown);
-    break;
-  default:
-    llvm_unreachable("Unhandled OpenMP directive");
-  }
+
+  // OMPD_unknown is only expected as the only region. If other regions
+  // are present OMPD_unknown should not be present.
+  assert((CaptureRegions[0] == OMPD_unknown ||
+          !llvm::is_contained(CaptureRegions, OMPD_unknown)) &&
+         "Misplaced OMPD_unknown");
 }
 
 bool clang::checkFailClauseParameter(OpenMPClauseKind FailClauseParameter) {

…z/spr/c03-capture-region

alexey-bataev · 2024-07-01T20:01:35Z

clang/lib/Basic/OpenMPKinds.cpp

-  case OMPD_distribute_simd:
-  case OMPD_scope:
-  case OMPD_dispatch:
+  auto getRegionsForLeaf = [&](OpenMPDirectiveKind LKind) {


Suggested change

auto getRegionsForLeaf = [&](OpenMPDirectiveKind LKind) {

auto GetRegionsForLeaf = [&](OpenMPDirectiveKind LKind) {

alexey-bataev · 2024-07-01T20:03:00Z

clang/lib/Basic/OpenMPKinds.cpp

+  // If no regions were added, then only the leafs using "unknown" were
+  // present. Push a single OMPD_unknown as the capture region.
+  if (CaptureRegions.empty())
    CaptureRegions.push_back(OMPD_unknown);


Do not like this code, this mayt hide bugs, better insert OPD_unknown explicitly where expected

I added a flag that is set when OMPD_unknown may be required. It's set only for the specific directives.

Why just don't add OMPD_unknown in lambda? This extra flag makes the code more complex and less readable.

Because these directives may be encountered as parts of a combined directive. If they push OMPD_unknown, we may end up with it together with other directives. We can do that, but then we'd have to strip the OMPD_unknown if there are other regions present.

For example, for OMPD_parallel_sections, we'd end up with [OMPD_parallel, OMPD_unknown].

clang/lib/Basic/OpenMPKinds.cpp

Co-authored-by: Alexey Bataev <[email protected]>

alexey-bataev

LG

Lands and disables: 9a7248a [clang][OpenMP] Rewrite `getOpenMPCaptureRegions` in terms of leafs (llvm#97110) │ needs more debugging. Change-Id: I10f94b3d96c3045eb7dfe4a76bcc76d776dc5c33

…lvm#97110) Replace the switch in `getOpenMPCaptureRegions` with a loop collecting capture regions based on the constituent directives. --------- Co-authored-by: Alexey Bataev <[email protected]>

kparzysz added 6 commits June 28, 2024 12:27

[clang][OpenMP] Implement isOpenMPCapturingDirective

0ee7c01

Check if the given directive can capture variables, and thus needs a captured statement. Simplify some code using this function.

fix typo

c901532

Fix wording

1d5ca2e

Merge branch 'users/kparzysz/spr/c01-is-exec' into users/kparzysz/spr…

b8bab8e

…/c02-is-capturing

[clang][OpenMP] Rewrite getOpenMPCaptureRegions in term of leafs

2d25e0d

Replace the switch in `getOpenMPCaptureRegions` with a loop collecting capture regions based on the constituent directives.

kparzysz requested review from alexey-bataev and mikerice1969 June 28, 2024 20:40

kparzysz changed the title ~~[clang][OpenMP] Rewrite getOpenMPCaptureRegions in term of leafs~~ [clang][OpenMP] Rewrite getOpenMPCaptureRegions in terms of leafs Jun 28, 2024

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" clang:openmp OpenMP related changes to Clang labels Jun 28, 2024

kparzysz added 3 commits July 1, 2024 08:17

Add assertion message

3753e4c

Merge branch 'main' into users/kparzysz/spr/c02-is-capturing

dbcaea7

Merge branch 'users/kparzysz/spr/c02-is-capturing' into users/kparzys…

617662b

…z/spr/c03-capture-region

alexey-bataev reviewed Jul 1, 2024

View reviewed changes

Capitalize name, add explicit check for the need of OMPD_unknown

338ba0b

Base automatically changed from users/kparzysz/spr/c02-is-capturing to main July 1, 2024 20:46

alexey-bataev reviewed Jul 1, 2024

View reviewed changes

clang/lib/Basic/OpenMPKinds.cpp Outdated Show resolved Hide resolved

clang/lib/Basic/OpenMPKinds.cpp Outdated Show resolved Hide resolved

clang/lib/Basic/OpenMPKinds.cpp Outdated Show resolved Hide resolved

kparzysz and others added 5 commits July 1, 2024 16:05

Update clang/lib/Basic/OpenMPKinds.cpp

4c167a2

Co-authored-by: Alexey Bataev <[email protected]>

Update clang/lib/Basic/OpenMPKinds.cpp

f6297d0

Co-authored-by: Alexey Bataev <[email protected]>

Update clang/lib/Basic/OpenMPKinds.cpp

73b712e

Co-authored-by: Alexey Bataev <[email protected]>

Delete unused declaration

a062a0e

Merge branch 'main' into users/kparzysz/spr/c03-capture-region

5c7f22f

alexey-bataev approved these changes Jul 1, 2024

View reviewed changes

kparzysz merged commit 9a7248a into main Jul 2, 2024
7 checks passed

kparzysz deleted the users/kparzysz/spr/c03-capture-region branch July 2, 2024 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][OpenMP] Rewrite `getOpenMPCaptureRegions` in terms of leafs #97110

[clang][OpenMP] Rewrite `getOpenMPCaptureRegions` in terms of leafs #97110

Uh oh!

kparzysz commented Jun 28, 2024

Uh oh!

llvmbot commented Jun 28, 2024

Uh oh!

alexey-bataev Jul 1, 2024

Uh oh!

kparzysz Jul 1, 2024

Uh oh!

alexey-bataev Jul 1, 2024

Uh oh!

kparzysz Jul 1, 2024

Uh oh!

alexey-bataev Jul 1, 2024

Uh oh!

kparzysz Jul 1, 2024

Uh oh!

kparzysz Jul 1, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexey-bataev left a comment

Uh oh!

Uh oh!

Uh oh!

	auto getRegionsForLeaf = [&](OpenMPDirectiveKind LKind) {
	auto GetRegionsForLeaf = [&](OpenMPDirectiveKind LKind) {

[clang][OpenMP] Rewrite getOpenMPCaptureRegions in terms of leafs #97110

[clang][OpenMP] Rewrite getOpenMPCaptureRegions in terms of leafs #97110

Uh oh!

Conversation

kparzysz commented Jun 28, 2024

Uh oh!

llvmbot commented Jun 28, 2024

Uh oh!

alexey-bataev Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

kparzysz Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

alexey-bataev Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

kparzysz Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

alexey-bataev Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

kparzysz Jul 1, 2024

Choose a reason for hiding this comment

Uh oh!

kparzysz Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alexey-bataev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

[clang][OpenMP] Rewrite `getOpenMPCaptureRegions` in terms of leafs #97110

[clang][OpenMP] Rewrite `getOpenMPCaptureRegions` in terms of leafs #97110

kparzysz Jul 1, 2024 •

edited

Loading