[Clang][OpenMP] Non-contiguous strided update #144635

amitamd7 · 2025-06-18T05:00:01Z

This patch handles the strided update in the #pragma omp target update from(data[a:b:c]) directive where 'c' represents the strided access leading to non-contiguous update in the data array when the offloaded execution returns the control back to host from device using the from clause.

Issue: Clang CodeGen where info is generated for the particular MapType (to, from, etc), it was failing to detect the strided access. Because of this, the MapType bits were incorrect when passed to runtime. This led to incorrect execution (contiguous) in the libomptarget runtime code.

Added a minimal testcase that verifies the working of the patch.

github-actions · 2025-06-18T05:00:18Z

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

llvmbot · 2025-06-18T05:00:48Z

@llvm/pr-subscribers-offload
@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Amit Tiwari (amitamd7)

Changes

This patch handles the strided update in the #pragma omp target update from(data[a:b:c]) directive where 'c' represents the strided access leading to non-contiguous update in the data array when the offloaded execution returns the control back to host from device using the from clause.

Issue: Clang CodeGen where info is generated for the particular MapType (to, from, etc), it was failing to detect the strided access. Because of this, the MapType bits were incorrect when passed to runtime. This led to incorrect execution (contiguous) in the libomptarget runtime code.

Added a minimal testcase that verifies the working of the patch.

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

2 Files Affected:

(modified) clang/lib/CodeGen/CGOpenMPRuntime.cpp (+34-1)
(added) offload/test/offloading/strided_update.c (+51)

diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index 4173355491fd4..81a2dd0fae5c9 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -7384,7 +7384,40 @@ class MappableExprsHandler {
     // dimension.
     uint64_t DimSize = 1;
 
-    bool IsNonContiguous = CombinedInfo.NonContigInfo.IsNonContiguous;
+    // Detects non-contiguous updates due to strided accesses.
+    // Sets the 'IsNonContiguous' flag so that the 'MapType' bits are set
+    // correctly when generating information to be passed to the runtime. The
+    // flag is set to true if any array section has a stride not equal to 1, or
+    // if the stride is not a constant expression (conservatively assumed
+    // non-contiguous).
+    bool IsNonContiguous = false;
+    for (const auto &Component : Components) {
+      const auto *OASE =
+          dyn_cast<ArraySectionExpr>(Component.getAssociatedExpression());
+      if (OASE) {
+        const Expr *StrideExpr = OASE->getStride();
+        if (StrideExpr) {
+          // Check if the stride is a constant integer expression
+          if (StrideExpr->isIntegerConstantExpr(CGF.getContext())) {
+            if (auto Constant =
+                    StrideExpr->getIntegerConstantExpr(CGF.getContext())) {
+              int64_t StrideVal = Constant->getExtValue();
+              if (StrideVal != 1) {
+                // Set flag if stride is not 1 (i.e., non-contiguous update)
+                IsNonContiguous = true;
+                break;
+              }
+            }
+          } else {
+            // If stride is not a constant, conservatively treat as
+            // non-contiguous
+            IsNonContiguous = true;
+            break;
+          }
+        }
+      }
+    }
+
     bool IsPrevMemberReference = false;
 
     bool IsPartialMapped =
diff --git a/offload/test/offloading/strided_update.c b/offload/test/offloading/strided_update.c
new file mode 100644
index 0000000000000..fc47216fb5684
--- /dev/null
+++ b/offload/test/offloading/strided_update.c
@@ -0,0 +1,51 @@
+// Checks that "update from" clause in OpenMP is supported when the elements are updated in a non-contiguous manner.
+// RUN: %libomptarget-compile-run-and-check-generic
+#include <omp.h>  
+#include <stdio.h>  
+  
+int main() {  
+  int len = 8;  
+  double data[len];  
+  #pragma omp target map(tofrom: len, data[0:len])  
+  {  
+    for (int i = 0; i < len; i++) {  
+      data[i] = i;  
+    }  
+  }  
+  // initial values  
+  printf("original host array values:\n");  
+  for (int i = 0; i < len; i++)  
+    printf("%f\n", data[i]);  
+  printf("\n");  
+  
+  #pragma omp target data map(to: len, data[0:len])  
+  {  
+    #pragma omp target  
+    for (int i = 0; i < len; i++) {  
+      data[i] += i ;  
+    }  
+  
+    #pragma omp target update from(data[0:8:2])  
+  }  
+  // from results  
+  // CHECK: 0.000000
+  // CHECK: 1.000000
+  // CHECK: 4.000000
+  // CHECK: 3.000000
+  // CHECK: 8.000000
+  // CHECK: 5.000000
+  // CHECK: 12.000000
+  // CHECK: 7.000000
+  // CHECK-NOT: 2.000000
+  // CHECK-NOT: 6.000000
+  // CHECK-NOT: 10.000000
+  // CHECK-NOT: 14.000000
+
+  printf("from target array results:\n");  
+  for (int i = 0; i < len; i++)  
+    printf("%f\n", data[i]);  
+  printf("\n");  
+  
+  return 0;  
+}  
+

alexey-bataev · 2025-06-18T14:10:06Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+    for (const auto &Component : Components) {
+      const auto *OASE =
+          dyn_cast<ArraySectionExpr>(Component.getAssociatedExpression());
+      if (OASE) {
+        const Expr *StrideExpr = OASE->getStride();
+        if (StrideExpr) {
+          // Check if the stride is a constant integer expression
+          if (StrideExpr->isIntegerConstantExpr(CGF.getContext())) {
+            if (auto Constant =
+                    StrideExpr->getIntegerConstantExpr(CGF.getContext())) {
+              int64_t StrideVal = Constant->getExtValue();
+              if (StrideVal != 1) {
+                // Set flag if stride is not 1 (i.e., non-contiguous update)
+                IsNonContiguous = true;
+                break;
+              }
+            }
+          } else {
+            // If stride is not a constant, conservatively treat as
+            // non-contiguous
+            IsNonContiguous = true;
+            break;
+          }
+        }
+      }
+    }


Please, convert it to lambda or a separate function

alexey-bataev · 2025-06-18T14:10:45Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+        const Expr *StrideExpr = OASE->getStride();
+        if (StrideExpr) {
+          // Check if the stride is a constant integer expression
+          if (StrideExpr->isIntegerConstantExpr(CGF.getContext())) {


Do you need this check before calling getIntegerConstantExpr?

Yes, because stride can be a variable/complex expression that is determined only at runtime, so conservatively treating it as non-contiguous.

You're answering wrong question. I'm asking if this check is required and is not covered by StrideExpr->getIntegerConstantExpr( already?

Okay, got it. No, it does not implicitly check if it is an integer.

I think getIntegerConstantExpr calls isIntegerConstantExpr, so you can skip it.

alexey-bataev · 2025-06-18T14:11:09Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+        if (StrideExpr) {
+          // Check if the stride is a constant integer expression
+          if (StrideExpr->isIntegerConstantExpr(CGF.getContext())) {
+            if (auto Constant =


Suggested change

if (auto Constant =

if (const auto Constant =

alexey-bataev · 2025-06-18T14:13:27Z

offload/test/offloading/strided_update.c

@@ -0,0 +1,51 @@
+// Checks that "update from" clause in OpenMP is supported when the elements are updated in a non-contiguous manner.


Also, need a clang unit test

alexey-bataev · 2025-06-18T14:21:47Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+              int64_t StrideVal = Constant->getExtValue();
+              if (StrideVal != 1) {


Suggested change

int64_t StrideVal = Constant->getExtValue();

if (StrideVal != 1) {

if (!Constant->isOne()) {

amitamd7 · 2025-06-26T12:09:36Z

Please ignore the indentation changes. I'll fix them soon in the revised version.

amitamd7 · 2025-07-15T10:20:28Z

The PR has incorporated the revised changes. Please, have a look.

alexey-bataev · 2025-07-17T11:45:31Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+      for (const auto &Component : Components) {
+        const auto *OASE =
+            dyn_cast<ArraySectionExpr>(Component.getAssociatedExpression());
+        if (OASE) {


if (!OASE) continue

alexey-bataev · 2025-07-17T11:46:00Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+          if (StrideExpr) {
+            if (const auto Constant =
+                    StrideExpr->getIntegerConstantExpr(CGF.getContext())) {
+              if (!Constant->isOne()) {
+                return true;
+              }


Use early exits to reduce strutured complexity

alexey-bataev · 2025-07-17T11:46:45Z

clang/include/clang/ASTMatchers/ASTMatchers.h

+extern const internal::VariadicDynCastAllOfMatcher<Stmt,
+                                                   OMPTargetUpdateDirective>
+    ompTargetUpdateDirective;


Why do you need this?

It specifically checks the omp target update directive. ompExecutableDirective would work too but its use case is generic.

The question is why do you need it in this patch?

Please, remove it here

alexey-bataev · 2025-07-17T11:47:02Z

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

@@ -4724,6 +4724,65 @@ void x() {
  EXPECT_TRUE(matchesWithOpenMP(Source8, Matcher));
 }

+TEST_P(ASTMatchersTest, OMPTargetUpdateDirective_IsStandaloneDirective) {


Clang tests are required

These tests better to move a separate patch, if really needed

The tests are moved to a separate patch: #150580

alexey-bataev · 2025-07-17T11:47:49Z

Also, update OpenMPSupport.rst and release notes

amitamd7 · 2025-07-23T11:47:24Z

Also, update OpenMPSupport.rst and release notes

alexey-bataev · 2025-07-23T14:19:51Z

clang/include/clang/ASTMatchers/ASTMatchers.h

+extern const internal::VariadicDynCastAllOfMatcher<Stmt,
+                                                   OMPTargetUpdateDirective>
+    ompTargetUpdateDirective;


The question is why do you need it in this patch?

alexey-bataev · 2025-07-23T14:20:11Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+        if (!OASE) {
+          continue;
+        }


Drop braces

alexey-bataev · 2025-07-23T14:20:19Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+        if (!StrideExpr) {
+          continue;
+        }


Drop braces

alexey-bataev · 2025-07-23T14:20:31Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+        if (!Constant) {
+          continue;
+        }
+        if (!Constant->isOne()) {
+          return true;
+        }


Drop extra braces

alexey-bataev · 2025-07-23T14:22:12Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+    // flag is set to true if any array section has a stride not equal to 1, or
+    // if the stride is not a constant expression (conservatively assumed
+    // non-contiguous).
+    bool IsNonContiguous = [&]() -> bool {


bool IsNonContiguous = CombinedInfo.NonContigInfo.IsNonContiguous || ... to avoid doing some extra work if CombinedInfo.NonContigInfo.IsNonContiguous is already set

Makes sense!

github-actions · 2025-07-25T07:29:36Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 HEAD --extensions h,cpp,c -- clang/test/OpenMP/target_update_strided_messages.c clang/test/OpenMP/target_update_strided_multiple_messages.c clang/test/OpenMP/target_update_strided_partial_messages.c offload/test/offloading/strided_multiple_update.c offload/test/offloading/strided_partial_update.c offload/test/offloading/strided_update.c clang/include/clang/ASTMatchers/ASTMatchers.h clang/lib/ASTMatchers/ASTMatchersInternal.cpp clang/lib/ASTMatchers/Dynamic/Registry.cpp clang/lib/CodeGen/CGOpenMPRuntime.cpp clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp clang/unittests/ASTMatchers/ASTMatchersNodeTest.cpp

View the diff from clang-format here.

diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index fee3f952b..e3d086b0a 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -7409,7 +7409,7 @@ private:
           return true;
         }
       }
-      return CombinedInfo.NonContigInfo.IsNonContiguous; 
+      return CombinedInfo.NonContigInfo.IsNonContiguous;
     }();
 
     bool IsPrevMemberReference = false;

amitamd7 · 2025-07-25T12:12:34Z

@alexey-bataev I think OMP target update directive and from clause declaration in clang unit-tests is independent from this patch. I have initiated a new PR for the same.

#150580

alexey-bataev · 2025-07-25T18:06:45Z

clang/lib/CodeGen/CGOpenMPRuntime.cpp

+        CombinedInfo.NonContigInfo.IsNonContiguous || [&]() -> bool {
+      for (const auto &Component : Components) {
+        const auto *OASE =
+            dyn_cast<ArraySectionExpr>(Component.getAssociatedExpression());
+        if (!OASE)
+          continue;
+
+        const Expr *StrideExpr = OASE->getStride();
+        if (!StrideExpr)
+          continue;
+
+        const auto Constant =
+            StrideExpr->getIntegerConstantExpr(CGF.getContext());
+        if (!Constant)
+          continue;
+
+        if (!Constant->isOne())
+          return true;
+      }
+      return false;
+    }();


.. || any_of(Components, [&](const auto &Component) {...};

alexey-bataev · 2025-07-25T18:07:14Z

clang/unittests/ASTMatchers/ASTMatchersNarrowingTest.cpp

@@ -4724,6 +4724,65 @@ void x() {
  EXPECT_TRUE(matchesWithOpenMP(Source8, Matcher));
 }

+TEST_P(ASTMatchersTest, OMPTargetUpdateDirective_IsStandaloneDirective) {


These tests better to move a separate patch, if really needed

llvmbot added clang Clang issues not falling into any other category clang:codegen IR generation bugs: mangling, exceptions, etc. clang:openmp OpenMP related changes to Clang offload labels Jun 18, 2025

saiislam requested review from alexey-bataev, jhuber6, saiislam and dhruvachak June 18, 2025 05:44

amitamd7 marked this pull request as draft June 18, 2025 05:57

alexey-bataev reviewed Jun 18, 2025

View reviewed changes

amitamd7 force-pushed the tiwari_target_non-contiguous_strided_update branch from 31b83e2 to 1383c0e Compare June 26, 2025 12:06

amitamd7 force-pushed the tiwari_target_non-contiguous_strided_update branch from 1383c0e to bdff136 Compare July 2, 2025 11:03

amitamd7 marked this pull request as ready for review July 15, 2025 10:18

amitamd7 requested a review from alexey-bataev July 15, 2025 10:18

alexey-bataev reviewed Jul 17, 2025

View reviewed changes

amitamd7 marked this pull request as draft July 21, 2025 15:32

amitamd7 marked this pull request as ready for review July 22, 2025 10:41

amitamd7 force-pushed the tiwari_target_non-contiguous_strided_update branch from bdff136 to 7f897ff Compare July 23, 2025 11:45

amitamd7 closed this Jul 23, 2025

amitamd7 reopened this Jul 23, 2025

alexey-bataev reviewed Jul 23, 2025

View reviewed changes

amitamd7 force-pushed the tiwari_target_non-contiguous_strided_update branch from 7f897ff to 9afb5a6 Compare July 25, 2025 11:58

alexey-bataev reviewed Jul 25, 2025

View reviewed changes

amitamd7 force-pushed the tiwari_target_non-contiguous_strided_update branch from 9afb5a6 to 2f4de4e Compare July 28, 2025 15:16

strided_update_offloading with lit-offload and clang-tests

940d6dc

amitamd7 force-pushed the tiwari_target_non-contiguous_strided_update branch from b95523d to 940d6dc Compare July 29, 2025 06:55

alexey-bataev approved these changes Jul 29, 2025

View reviewed changes

		@@ -0,0 +1,51 @@
		// Checks that "update from" clause in OpenMP is supported when the elements are updated in a non-contiguous manner.

		int64_t StrideVal = Constant->getExtValue();
		if (StrideVal != 1) {

	int64_t StrideVal = Constant->getExtValue();
	if (StrideVal != 1) {
	if (!Constant->isOne()) {

[Clang][OpenMP] Non-contiguous strided update #144635

Are you sure you want to change the base?

[Clang][OpenMP] Non-contiguous strided update #144635

Uh oh!

Conversation

amitamd7 commented Jun 18, 2025

Uh oh!

github-actions bot commented Jun 18, 2025

Uh oh!

llvmbot commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amitamd7 commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amitamd7 commented Jul 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexey-bataev commented Jul 17, 2025

Uh oh!

amitamd7 commented Jul 23, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 25, 2025

Uh oh!

amitamd7 commented Jul 25, 2025

Uh oh!

Choose a reason for hiding this comment

llvmbot commented Jun 18, 2025 •

edited

Loading

amitamd7 commented Jun 26, 2025 •

edited

Loading