[Clang][OpenMP] Handle check for non-contiguous mapping in pointer-based array sections

[amitamd7]

1. ElementType deduction for pointer-based array sections

Problem: Pointer-based array sections were previously ignored during ElementType deduction, leading to incorrect assumptions about array item types.

This often resulted in out-of-bounds access, as seen in the assertion failure:

Assertion `idx < size()' failed.
llvm-project/llvm/include/llvm/ADT/SmallVector.h:292:
reference llvm::SmallVectorTemplateCommon<llvm::Value *>::operatorsize_type
[T = llvm::Value *]

Fix: Added a check in clang/lib/CodeGen/CGOpenMPRuntime.cpp to ensure ElementType is correctly detected for cases involving non-contiguous updates with a base pointer.
Impact: Resolves failures in OpenMP_VV (formerly sollve_vv) and other offload/clang-OpenMP tests:

All tests under:
https://github.com/OpenMP-Validation-and-Verification/OpenMP_VV/tree/master/tests/5.0/target_update

test_target_update_mapper_from_discontiguous.c
test_target_update_mapper_to_discontiguous.c
test_target_update_to_discontiguous.c
test_target_update_from_discontiguous.c

2. Zero-dimension propagation in struct member mappings

Problem: A zero-dimension entry for struct members introduced inconsistencies in complex mapping logic within OMPIRBuilder.cpp.

Placeholder zeros propagated to emitNonContiguousDescriptor(), breaking reverse indexing logic and corrupting IR:

Loops assume Dims[I] >= 1. When Dims[I] == 0:

Reverse indexing still stores pointers to uninitialized allocas or mismatched slots. Runtime interprets ArgSizes[I] (derived from Dims[I]) as dimensionality, causing size/offset calculations to collapse to zero → results in size=0 async copy and plugin interface errors.

Fix: Prepend a synthetic dimension of size 1 instead of appending a zero, preserving correctness in targetDataUpdate() for non-contiguous updates.
Impact: Added dedicated test cases that previously failed on main.

[jtb20]

Does this patch work for the test_target_update_mapper_{to,from}_discontiguous tests?

[amitamd7]

Does this patch work for the test_target_update_mapper_{to,from}_discontiguous tests?

Yes Julian, for the compilation error marked in SWDEV-553534

[jtb20]

I had a quick look because I was looking into this briefly too -- it still crashes for me, like so:

Thread 1 "test_target_upd" received signal SIGSEGV, Segmentation fault.
0x00007ffff7aec887 in targetDataUpdate (Loc=0x55555555b5c8, Device=..., ArgNum=2, ArgsBase=0x7fffffffd880, Args=0x7fffffffd870, ArgSizes=0x7fffffffd860, ArgTypes=0x5555555582d0, ArgNames=0x55555555b5b8, ArgMappers=0x0, AsyncInfo=..., AttachInfo=0x0, FromMapper=false) at /work1/julbrown/code/mainline+amd-staging/src/llvm-project-main/offload/libomptarget/omptarget.cpp:1297
1297              NonContig[DimSize - 1].Count * NonContig[DimSize - 1].Stride;

The strange thing about the surrounding loop (in generateInfoForComponentList) is that it appears like it was written to handle this case at some point, see e.g. the comment:

        // If ElementType is null, then it means the base is a pointer
        // (neither CAT nor VAT) and we'll attempt to get ElementType again
        // for next iteration.

But, that can't ever happen because of the if stmt further up:

      if (!OASE)
        continue;

So it might be worth doing some archaeology to figure out why/when this was broken.

JFYI!

[amitamd7]

Yes indeed the if (!OASE) check is FALSE only once (when it is an single dimension array-section expression) and we would ideally want to get the ElementType to be deduced in that traversal itself for OASE case. I checked few cases where it hits off CAT, VAT while the one in test_target_update_mapper_from_discontiguous.c (and other mentioned tests in the ticket), it goes undetected.

[llvmbot]

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

@llvm/pr-subscribers-clang-codegen

Author: Amit Tiwari (amitamd7)

Changes

This patch adds the check where pointer-based array sections are not considered while deducing ElementType of the array items.

---

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

1 Files Affected:

• (modified) clang/lib/CodeGen/CGOpenMPRuntime.cpp (+6-4)

diff --git a/clang/lib/CodeGen/CGOpenMPRuntime.cpp b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
index b38eb54036e60..416a8c1ad4a03 100644
--- a/clang/lib/CodeGen/CGOpenMPRuntime.cpp
+++ b/clang/lib/CodeGen/CGOpenMPRuntime.cpp
@@ -7930,10 +7930,12 @@ class MappableExprsHandler {
           ElementType = CAT->getElementType().getTypePtr();
         else if (VAT)
           ElementType = VAT->getElementType().getTypePtr();
-        else
-          assert(&Component == &*Components.begin() &&
-                 "Only expect pointer (non CAT or VAT) when this is the "
-                 "first Component");
+        else if (&Component == &*Components.begin()) {
+          // Handle pointer-based array sections like data[a:b:c]
+          if (const auto *PtrType = Ty->getAs<PointerType>()) {
+            ElementType = PtrType->getPointeeType().getTypePtr();
+          }
+        }
         // If ElementType is null, then it means the base is a pointer
         // (neither CAT nor VAT) and we'll attempt to get ElementType again
         // for next iteration.

[efriedma-quic]

Missing testcase.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[amitamd7]

Missing testcase.

Done.

[amitamd7]

Missing testcase.

Done.

[alexey-bataev]

Drop braces

[alexey-bataev]

Clang unit test does not need to use these functions, remove

[amitamd7]

CI/CD doesn't support the malloc declaration used in the unit-test. Specifying the declaration as a pointer to <TYPE> is the integral part of the test. The use of extern can be seen in other tests too, when the type specified mismatches with the dynamically linked malloc type. Without the malloc definition, the build CI/CD fails so:

# |   File /home/gha/actions-runner/_work/llvm-project/llvm-project/clang/test/OpenMP/target_update_strided_ptr_messages_to.c Line 39: call to undeclared library function 'free' with type 'void (void *)'; **ISO C99 and later do not support implicit function declarations**
 # | error: 'expected-note' diagnostics seen but not expected: 
 # |   File /home/gha/actions-runner/_work/llvm-project/llvm-project/clang/test/OpenMP/target_update_strided_ptr_messages_to.c Line 6: include the header <stdlib.h> or explicitly provide a declaration for 'malloc' 

[alexey-bataev]

You do not need to use them at all, this is a codegen test, not the execution, you can simply remove them

[amitamd7]

Fixed.

[alexey-bataev]

Better not to remove these checks, but change accordingly

[amitamd7]

Added few more clang-unit tests that support the inlined arrays in a structure. Corresponding offload lit-tests were added previously.

[alexey-bataev]

Not recommended to use %.offload_baseptrs-like names, replace by regexps

[amitamd7]

Fixed.