[Openmp-commits] [clang] [llvm] [openmp] [CLANG][OpenMP] Restore loop variable values after transformation directives (PR #214717)

[Zahira Ammarguellat via Openmp-commits]

zahiraam wrote:

> Check these tests:
> 
> ```
> // Loop variables of transformation-affected loops must have the values
> // they would have without the directive (OpenMP 6.0 loop transformation
> // finalization rule). Each *_ref function is the untransformed reference;
> // the transformed twin must print the same values.
> //
> // Build with a compiler that has the PR applied, run, compare against the
> // CHECK lines. The tile/stripe rows currently produce j=5 on the PR:
> // the restore of an inner dimension fires even when the enclosing
> // dimension has zero iterations, but without the directive the inner loop
> // would not have run at all, so j must be left untouched. The interchange
> // row is left without a CHECK: the PR review ruled the shared case gets
> // the exit value (3); confirm that this also covers tile/stripe.
> #include <stdio.h>
> 
> void tile_ref(int n) {
>   int i, j;
>   i = 77;
>   j = 111;
>   for (i = 0; i < n; i++)
>     for (j = 0; j < 5; j++)
>       ;
>   printf("tile_ref i=%d j=%d\n", i, j);
> }
> 
> void tile(int n) {
>   int i, j;
>   i = 77;
>   j = 111;
> #pragma omp tile sizes(2, 3)
>   for (i = 0; i < n; i++)
>     for (j = 0; j < 5; j++)
>       ;
>   printf("tile i=%d j=%d\n", i, j);
> }
> 
> void stripe_ref(int n) {
>   int i, j;
>   i = 77;
>   j = 111;
>   for (i = 0; i < n; i++)
>     for (j = 0; j < 5; j++)
>       ;
>   printf("stripe_ref i=%d j=%d\n", i, j);
> }
> 
> void stripe(int n) {
>   int i, j;
>   i = 77;
>   j = 111;
> #pragma omp stripe sizes(2, 3)
>   for (i = 0; i < n; i++)
>     for (j = 0; j < 5; j++)
>       ;
>   printf("stripe i=%d j=%d\n", i, j);
> }
> 
> void interchange_ref(int n) {
>   int i, j;
>   i = 77;
>   j = 111;
>   for (i = 0; i < n; i++)
>     for (j = 0; j < 3; j++)
>       ;
>   printf("interchange_ref i=%d j=%d\n", i, j);
> }
> 
> void interchange(int n) {
>   int i, j;
>   i = 77;
>   j = 111;
> #pragma omp interchange
>   for (i = 0; i < n; i++)
>     for (j = 0; j < 3; j++)
>       ;
>   printf("interchange i=%d j=%d\n", i, j);
> }
> 
> void tile_full_ref(void) {
>   int i, j;
>   i = 77;
>   j = 111;
>   for (i = 0; i < 6; i++)
>     for (j = 0; j < 5; j++)
>       ;
>   printf("tile_full_ref i=%d j=%d\n", i, j);
> }
> 
> void tile_full(void) {
>   int i, j;
>   i = 77;
>   j = 111;
> #pragma omp tile sizes(2, 3)
>   for (i = 0; i < 6; i++)
>     for (j = 0; j < 5; j++)
>       ;
>   printf("tile_full i=%d j=%d\n", i, j);
> }
> 
> int main(void) {
>   tile_ref(0);
>   tile(0);
>   stripe_ref(0);
>   stripe(0);
>   interchange_ref(0);
>   interchange(0);
>   tile_full_ref();
>   tile_full();
>   return 0;
> }
> 
> // Spec-correct output; the failing rows show the issue on the PR:
> // CHECK: tile_ref i=0 j=111
> // CHECK-NOT: tile i=0 j=5
> // CHECK: tile i=0 j=111
> // CHECK: stripe_ref i=0 j=111
> // CHECK-NOT: stripe i=0 j=5
> // CHECK: stripe i=0 j=111
> // CHECK: interchange_ref i=0 j=111
> // interchange row: PR review ruled the shared case gets the exit value;
> // verify the ruling covers tile/stripe as well.
> // CHECK: tile_full_ref i=6 j=5
> // CHECK: tile_full i=6 j=5
> ```
> 
> ```
> // Restore statements after a fused loop must touch user variables only.
> // The internal fused-loop index is compiler generated, is never read after
> // the construct, and must not get a restoring store. The nested fuse here
> // makes the outer fuse collect the inner fused loop's helpers, whose
> // counter is that internal index.
> // RUN: %clang_cc1 -fopenmp -fopenmp-version=60 -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
> 
> extern "C" void nested_fuse(void) {
> #pragma omp fuse looprange(2, 2)
>   {
> #pragma omp fuse looprange(2, 2)
>     {
>       for (int i = 0; i < 8; ++i)
>         ;
>       for (int j = 0; j < 16; ++j)
>         ;
>       for (int k = 0; k < 32; ++k)
>         ;
>     }
>     int l;
>     for (l = 0; l < 4; ++l)
>       ;
>     int m;
>     for (m = 0; m < 6; ++m)
>       ;
>   }
> }
> 
> // After the outer fused loop: j and k are restored by the inner fuse, l by
> // this fuse. Nothing between those stores may write an internal fuse index.
> // On the PR there is a guarded block assigning .omp_fuse_index here, dead
> // code visible as a failed CHECK-NOT.
> // CHECK: store i32 16, ptr %j, align 4
> // CHECK: store i32 32, ptr %k, align 4
> // CHECK-NOT: store i32 {{.*}}, ptr %".omp_fuse_index{{[0-9]*}}"
> // CHECK: store i32 4, ptr %l, align 4
> ```
> 
> ```
> // A range-based for is a canonical OpenMP loop; its control iterator must
> // be restored after a fuse like any other loop variable. The plain for
> // with a pointer control variable in the same sequence is restored, the
> // range-for iterator currently is not.
> // RUN: %clang_cc1 -fopenmp -fopenmp-version=60 -triple x86_64-unknown-linux-gnu -emit-llvm -o - %s | FileCheck %s
> 
> extern "C" void fuse_range_for(void) {
>   int arr[8];
>   int t;
>   int *q;
> #pragma omp fuse
>   {
>     for (t = 0; t < 4; ++t)
>       ;
>     for (q = arr; q != arr + 8; ++q)
>       ;
>     for (int &x : arr)
>       ;
>   }
> }
> 
> // CHECK-LABEL: define {{.*}}@fuse_range_for
> // CHECK: store i32 4, ptr %t, align 4
> // // CHECK: store ptr {{.*}}, ptr %q, align 8
> // CHECK: getelementptr inbounds i32, ptr %arr, i64 8
> // CHECK: store ptr {{.*}}, ptr %q, align 8
> // The range-for iterator is restored to one past arr, as under tile
> // below; that store is missing on the PR and this CHECK exposes it.
> // CHECK: store ptr {{.*}}, ptr %__begin
> 
> extern "C" void tile_range_for(void) {
>   int arr[8];
> #pragma omp tile sizes(2)
>   for (int &x : arr)
>     ;
> }
> 
> // CHECK-LABEL: define {{.*}}@tile_range_for
> // Control proving the helper expressions carry a valid restore for a
> // range-for; if this CHECK also fails, the skip is not specific to the
> // fuse filter.
> // CHECK: store ptr {{.*}}, ptr %__begin
> ```
> 
> ```
> // Class-type loop variables are excluded from the restoration because the
> // builder only handles arithmetic and pointer counters. The finalization
> // rule covers them the same way: after the construct the control iterator
> // must equal end().
> // Build with a compiler that has the PR applied and run; compare against
> // the expected output below. The PR prints 0.
> #include <vector>
> #include <cstdio>
> 
> int main() {
>   std::vector<int> v(6);
>   std::vector<int>::iterator it;
> 
>   it = v.begin();
>   for (it = v.begin(); it != v.end(); ++it)
>     ;
>   std::printf("plain at_end=%d\n", it == v.end());
> 
>   it = v.begin();
> #pragma omp tile sizes(2)
>   for (it = v.begin(); it != v.end(); ++it)
>     ;
>   std::printf("tile at_end=%d\n", it == v.end());
> 
>   return 0;
> }
> 
> // Spec-correct output; the failing row shows the issue on the PR:
> // CHECK: plain at_end=1
> // CHECK-NOT: tile at_end=0
> // CHECK: tile at_end=1
> ```

Test 1-3 pass. Test 4 segfaults with clang with and without this PR, https://godbolt.org/z/PKaa4nTxv .
`Class-iterator` finalization stays as the FIXME; I'll fix it together with the crash in a follow-up.

https://github.com/llvm/llvm-project/pull/214717