[Flang][openmp]Fix crash in OpenMP semantic check( bug 48308)

[sameeranjoshi]

Summary

Fixes the bug reported in https://bugs.llvm.org/show_bug.cgi?id=48308

[clementval]

The title doesn't match the fix. You are fixing smth in name resolution and not semantic checking.

[tskeith]

flang/test/Semantics/bug48308.f90

1 ↗

(On Diff #309470)

I don't like the name of this test. The name should indicate what functionality it is testing, like every other test.

[sameeranjoshi]

flang/test/Semantics/bug48308.f90

1 ↗

(On Diff #309470)

I was thinking similar to the way GNU compiler captures them (pr48308.f90) so as to find the test quickly from the name on bugzilla.
Will add a better name.

[sameeranjoshi]

change test name.
Change title.

[clementval]

flang/lib/Semantics/resolve-directives.cpp

878

Not specially for this patch but just a quick question: What is the ultimate standard we are targeting in Flang? 4.5 or 5.0. I have seen both in new patches and since there is no way to enforce one standard or another it feel weird to mix them. Maybe I missed this discussion but if you have any information it would be great for my personal knowledge.

883

First line reference 4.5 and here 5.0 .... I think it would be nice to have a homogeneity here.

[kiranchandramohan]

flang/lib/Semantics/resolve-directives.cpp

878

Arm's immediate requirement is to replace the existing Flang (flang-compiler/flang) compiler with llvm/flang. The current compiler has partial support for 4.5 and no support for 5.0. Hence you see commits from our side referring to 4.5. Once we are done with checks for 4.5 we will do an upgrade to 5.0. AMD and others will continue to work on 5.0.

I agree that there is weirdness in this. But I anticipate quick progress on semantic checks and this will go away in a few months time.

883

I think specific point might not be from the OpenMP standard. It is probably a general comment that do while loops might not have iteration variables.

But the ones below are forward looking comments.

[clementval]

flang/lib/Semantics/resolve-directives.cpp

878

Thanks for the clarification @kiranchandramohan. Is this written somewhere in the doc? If not, it would maybe be nice to have just a line or two in the documentation just mentioning this "transition" phase.

883

Ok. Just very confusing to have 3 versions of the standard on the same comment blocks.

897

Are these tabs?

[kiranktp]

flang/lib/Semantics/resolve-directives.cpp

883

As Valentine mentioned, having different standards mentioned under one construct will be very confusing. It would be good to remove references to 5.0 and 5.1 here. As and when we start migrating the next OpenMP version, we can modify these appropriately.

[kiranktp]

flang/lib/Semantics/resolve-directives.cpp

878

This is pending for quite sometime. We should add OpenMP 5.0 grammar file as well.
Would it make sense to modify "OpenMP 4.5 Grammar" page to "OpenMP Grammar" and add details about current implementation and future transition along with both grammar[4.5 and 5.0]?

[sameeranjoshi]

flang/lib/Semantics/resolve-directives.cpp

883

I think specific point might not be from the OpenMP standard. It is probably a general comment that do while loops might not have iteration variables.

But the ones below are forward looking comments.

Yes this is correct.
Indeed it would have been confusing when actually changing something related to code in that part, if I wouldn't have mentioned references as
Until 5.0 usage of loops apart from do loop is not well specified.
In 5.1 there are provisions for one of the forms of loops(do concurrent).
And the reference for comment related to function was already present but inorder to find 2.15.1.1 you needed to explicitly mention the 4.5 standard as there are many references to standard.

[sameeranjoshi]

Try to simplify comments.

[kiranchandramohan]

LGTM.

[clementval]

Thanks for updating the comment. It's clearer now

flang/lib/Semantics/resolve-directives.cpp

if (auto *symbol{ResolveOmp(iv, Symbol::Flag::OmpPrivate, targetIt->scope)}) {
     symbol->set(Symbol::Flag::OmpPreDetermined);
     iv.symbol = symbol; // adjust the symbol within region
     for (auto it{dirContext_.rbegin()}; it != targetIt; ++it) {
       AddToContextObjectWithDSA(*symbol, Symbol::Flag::OmpPrivate, *it);
     }
   }
 }
 
-// 2.15.1.1 Data-sharing Attribute Rules - Predetermined
+// [OMP-4.5]2.15.1.1 Data-sharing Attribute Rules - Predetermined
 //   - A loop iteration variable for a sequential loop in a parallel
 //     or task generating construct is private in the innermost such
 //     construct that encloses the loop
+// Loop iteration variables are not well defined for DO WHILE loop.
+// Use of DO CONCURRENT inside OpenMP construct is unspecified behavior
+// till OpenMP-5.0 standard.
+// In above both cases we skip the privatization of iteration variables.
 bool OmpAttributeVisitor::Pre(const parser::DoConstruct &x) {
+  // TODO:[OpenMP 5.1] DO CONCURRENT indices are private
+  if (x.IsDoNormal()) {
     if (!dirContext_.empty() && GetContext().withinConstruct) {
       if (const auto &iv{GetLoopIndex(x)}; iv.symbol) {
         if (!iv.symbol->test(Symbol::Flag::OmpPreDetermined)) {
           ResolveSeqLoopIndexInParallelOrTaskConstruct(iv);
         } else {
           // TODO: conflict checks with explicitly determined DSA
         }
       }
     }
+  }
   return true;
 }
 
 std::int64_t OmpAttributeVisitor::GetAssociatedLoopLevelFromClauses(
     const parser::OmpClauseList &x) {
   std::int64_t orderedLevel{0};
   std::int64_t collapseLevel{0};
 

flang/test/Semantics/omp-no-dowhile-in-parallel.f90

+! RUN: %S/test_errors.sh %s %t %f18 -fopenmp
+
+subroutine bug48308(x,i)
+  real :: x(:)
+  integer :: i
+  !$omp parallel firstprivate(i)
+    do while (i>0)
+      x(i) = i
+      i = i - 1
+    end do
+  !$omp end parallel
+end subroutine
+
+subroutine s1(x,i)
+  real :: x(:)
+  integer :: i
+  !$omp parallel firstprivate(i)
+    do i = 10, 1, -1
+      x(i) = i
+    end do
+  !$omp end parallel
+
+  !$omp parallel firstprivate(i)
+    do concurrent (i = 1:10:1)
+      x(i) = i
+    end do
+  !$omp end parallel
+end subroutine