[PATCH] D108904: [flang][OpenMP] Added semantic checks for sections (associated section(s) should be structured block(s)) and simd constructs (associated loop(s) should be structured block(s))

[Peixin Qiao via Phabricator via llvm-commits]

peixin added inline comments.


================
Comment at: flang/test/Semantics/omp-simd01.f90:14
   do i = 1, 10
     do j = 1, 10
       print *, "omp simd"
----------------
NimishMishra wrote:
> peixin wrote:
> > NimishMishra wrote:
> > > peixin wrote:
> > > > Does your implementation support the following code:
> > > > 
> > > > ```
> > > > !$omp simd
> > > > do i = 1, 10
> > > >   30 stop
> > > >   do j = 1, 10
> > > >     goto 30
> > > >   end do
> > > > end do
> > > > !$omp end simd.
> > > > ```
> > > > If no collapse clause specified, it behaves as if only one loop immediately following is associated with the construct. Could you please add the test in your test case?
> > > The present state of the implementation gives a '//STOP statement is not allowed in a SIMD construct//'. Is that what you were looking for?
> > > 
> > > If not, this was added as //void Post(const parser::StopStmt &) { EmitBranchOutError("STOP"); }// in //NoBranchingEnforce// (source: https://reviews.llvm.org/D88655). Possibly @kiranchandramohan can offer some insights. According to his comment there, parallel region can have a STOP. I am not sure about SIMD.
> > > 
> > > 
> > > That said, if I replace that STOP statement with something else (like the example below), no errors are generated. That is expected I believe since branching within SIMD's associated DO loop shouldn't be considered a violation of the structured block restriction.
> > > 
> > > 
> > > ```
> > >  program sample                                                                                                                                                 use omp_lib                                                                                                                                                !$omp simd                                                                                                                                                   do i = 1, 10                                                                                                                                                 30 print*, "sample"                                                                                                                                          do j = 1, 10                                                                                                                                                   goto 30                                                                                                                                                      end do                                                                                                                                                     end do                                                                                                                                                     !$omp end simd                                                                                                                             end program sample
> > > ```
> > You can replace it with print *, "sample". My key point is to check branching out to the nested loop which is not associated with `simd` construct. I thought out one more better test case as follows:
> > 
> > ```
> > !$omp simd collapse(2)
> > do i = 1, 10
> >   30 print *, "This is wrong"
> >   do j = 1, 10
> >     40 print *, "This is OK"
> >     do k = 1, 10
> >       if (expression) then
> >         goto 30
> >       end if
> >       if (expression2) then
> >         goto 40
> >       end if
> >     end do
> >   end do
> > end do
> > !$omp end simd
> > ```
> > If my understanding is correct, please polish this test case and add it.
> The constraint is there, albeit not because of my changes. When I run this case, it gives a //The value of the parameter in the COLLAPSE or ORDERED clause must not be larger than the number of nested loops following the construct. //
> 
> The reason for this, as it seems to me is, that when we give a //COLLAPSE(2)//, it checks for immediately nesting of exactly 2 DO constructs. Anything else simply raises this error. In this vein, the following modification of this test case succeeds:
> 
> 
> ```
> !$omp simd collapse(2)
> do i = 1, 10  ! Removed 30 print*, ...
>   do j = 1, 10
>     40 print *, "This is OK"
>     do k = 1, 10
>       if (expression) then
>         goto 30
>       end if
>       if (expression2) then
>         goto 40
>       end if
>     end do
>   end do
> end do
> !$omp end simd
> ```
> 
> Similarly, trying //COLLAPSE(3)// would force removal of the print statement at label 40. So I think we can be reasonably assured that the simd nesting checks implemented in //check-omp-structure.cpp// will assure absence of any such statements which can violate the structured block restriction.
> 
> Please let me know your thoughts on this.
Thanks for the investigation. OK to me now.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D108904/new/

https://reviews.llvm.org/D108904