Page MenuHomePhabricator

[flang][OpenMP] Add semantic check for occurrence of variables other than loop iteration variable in a `linear` clause associated with a `distribute` construct.
AcceptedPublic

Authored by arnamoy10 on Apr 9 2021, 12:17 PM.

Details

Summary

Implement the following semantic check:

"A list item may not appear in a linear clause, unless it is the loop iteration variable."

Diff Detail

Unit TestsFailed

TimeTest
170 msx64 windows > LLVM.Other::new-pm-lto-defaults.ll
Script: -- : 'RUN: at line 5'; c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\opt.exe -disable-verify -verify-cfg-preserved=0 -debug-pass-manager -passes='lto<O1>' -S C:\ws\w16e2-1\llvm-project\premerge-checks\llvm\test\Other\new-pm-lto-defaults.ll 2>&1 | c:\ws\w16e2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16e2-1\llvm-project\premerge-checks\llvm\test\Other\new-pm-lto-defaults.ll --check-prefix=CHECK-O --check-prefix=CHECK-O1

Event Timeline

arnamoy10 created this revision.Apr 9 2021, 12:17 PM
arnamoy10 requested review of this revision.Apr 9 2021, 12:17 PM
Herald added a project: Restricted Project. · View Herald Transcript
kiranchandramohan requested changes to this revision.Apr 12 2021, 12:42 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
490

You have to check whether the nested loops are associated with the distribute simd construct. If they are associated then they should be picked up, otherwise they should not be. Association here is probably only through the collapse clause.

For e.g: The following test should provide an error.

!$omp distribute simd linear(i,j) 
do i = 1, N
  do j = 1, N
     a = 3.14
  enddo
enddo
!$omp end distribute simd

The following test should not.

!$omp distribute simd collapse(2) linear(i,j) 
 do i = 1, N
   do j = 1, N
      a = 3.14
   enddo
 enddo
 !$omp end distribute simd
This revision now requires changes to proceed.Apr 12 2021, 12:42 AM
arnamoy10 updated this revision to Diff 338224.EditedApr 16 2021, 1:49 PM

Update as per reviewers' comments to check "association" of loops with the distribute construct based on the value in the collapse clause.

Also added a test case.

kiranchandramohan requested changes to this revision.Sat, Apr 24, 2:51 PM

Looks mostly OK. One change request and a few nits.

flang/lib/Semantics/check-omp-structure.cpp
295

The restriction applies to only the following two cases,

Directive::OMPD_distribute_parallel_do_simd
Directive::OMPD_distribute_simd

For Directive::OMPD_distribute, linear is not in the list of clauses.
For Directive::OMPD_distribute_parallel_do no linear clause can be specified.

478

Nit: spelling collapse.

481–488

Nit: There is a function GetOrdCollapseLevel which you can possibly use.

537

Nit: at -> in?

flang/lib/Semantics/check-omp-structure.h
82–83

Nit: Are these two fors required? Do these apply for Fortran?

This revision now requires changes to proceed.Sat, Apr 24, 2:51 PM

Addressing reviewers comments:

  1. Change the directive set to check for
  2. Use of existing functions to get the collapse level.
  3. Minor typo fixes.
flang/lib/Semantics/check-omp-structure.cpp
481–488

Thanks, modified.

537

Thanks, modified

flang/lib/Semantics/check-omp-structure.h
82–83

Thanks, removed..

kiranchandramohan requested changes to this revision.Thu, Apr 29, 11:01 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/check-omp-structure.cpp
477

Would it be possible to go through the linear clause first? If there is no linear clause there is no need to go through the iteration variables.

This revision now requires changes to proceed.Thu, Apr 29, 11:01 AM
flang/lib/Semantics/check-omp-structure.cpp
477

If you do this check in Leave then you can probably use FindClause/FindClauses.

arnamoy10 added inline comments.Wed, May 5, 2:18 PM
flang/lib/Semantics/check-omp-structure.cpp
477

Unfortunately, the check cannot be done in Leave(), because EndLoopDirective is Enter()-ed before, which erases the clauselist using ResetPartialContext(). So FindClause/FindClauses cannot retrieve the clause list anymore

So checking for linear clause first also is not helpful here.

flang/lib/Semantics/check-omp-structure.cpp
477

OK.
What do you think about the following?
Collect all symbols in the linear clause in linearSet. If not empty, go through the associated loops and remove iteration variable symbols from the linearSet. For anything remaining in the linearSet, print errors.
This will only be a rearrangement of the current code.

Updating the algorithm as per suggestion, to skip collection of loop induction variable if no linear clause is found

flang/lib/Semantics/check-omp-structure.cpp
477

Thanks, updated as per suggestion.

LGTM. I have a nit suggestion.

flang/lib/Semantics/check-omp-structure.cpp
504

Nit: I think early returns are only sparingly used in the flang code.

This revision is now accepted and ready to land.Fri, May 14, 3:50 PM