Page MenuHomePhabricator

[flang][OpenMP] Support for Collapse
ClosedPublic

Authored by Leporacanthicus on May 10 2022, 3:45 AM.

Details

Summary

Convert Fortran parse-tree into MLIR for collapse-clause.

Includes simple Fortran to LLVM-IR test, with auto-generated
check-lines (some of which have been edited by hand).

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 10 2022, 3:45 AM
Leporacanthicus requested review of this revision.May 10 2022, 3:45 AM

Looks OK. A few Nit comments.

flang/lib/Lower/OpenMP.cpp
140

Nit: I think you are introducing an auto here. The previous state (without the auto) is better.

408

Did you accidentally remove this assert. I think bounds are required for a worksharing loop. This might be a change from fir-dev.

453

Nit: I prefer the previous formatting.

466

Nit: 2->3

LGTM except for @kiranchandramohan 's comments.

flang/lib/Lower/Bridge.cpp
1423

nit

flang/test/Lower/OpenMP/omp-wsloop-collapse.f90
5

Nit: Can you write these in one line and remove the check-prefix?

! RUN: bbc -fopenmp -emit-fir %s -o - | FileCheck %s

A few nits. Thanks for sending this.

flang/lib/Lower/OpenMP.cpp
138

It would be nice to know what the argument "args" refers to, it is very generic. Can we maybe please add a description of arguments over this function as a comment?

409

Can we please move this up? Right now there is (almost) a very nice separation of clauses in the upper half of the file and constructs in the lower half.

flang/test/Lower/OpenMP/omp-wsloop-collapse.f90
53

nit: end do?

peixin added inline comments.May 11 2022, 6:49 AM
flang/test/Lower/OpenMP/omp-wsloop-collapse.f90
53

end do is totally the same as enddo for Fortran code. Both of them are commonly used in HPC workloads.

Leporacanthicus marked 6 inline comments as done.

Update for review comments

flang/lib/Lower/OpenMP.cpp
138

I have added a comment. I'm not sure the comment is THAT helpful, because I only know roughly the overall purpose of this function in the first place - I just changed arg to args and loop over the vector instead of

flang/test/Lower/OpenMP/omp-wsloop-collapse.f90
53

I agree with both comments: end do and enddo1 both works. I prefer end do - not sure why it wasn't in the first place - I have changed it - all other tests in Lower/OpenMP use that form.

There are enddo in Semantics tests.

[Total in flang/test enddo: 205, end do: 1132 - no, I'm not volunteering to unify this, I just wanted to see if one was more common than the other]

peixin accepted this revision.May 12 2022, 11:57 PM

LGTM

flang/test/Lower/OpenMP/omp-wsloop-collapse.f90
10

BTW, which script file did you use to generate these FIR checks?

This revision is now accepted and ready to land.May 12 2022, 11:57 PM

It seems you missed a comment. Otherwise LGTM.

flang/lib/Lower/OpenMP.cpp
408

Don't know whether you missed this comment or prefer not to change.

The OpenMP standard says the following, so i think the assert is better.
"The do-loop cannot be a DO WHILE or a DO loop without loop control."

shraiysh accepted this revision.May 13 2022, 11:57 AM

LGTM with comments addressed. Thanks!

flang/lib/Lower/OpenMP.cpp
408

+1. Maybe graceful error out during semantic checks for this (not necessarily in this patch) plus an assert here should be added.

LGTM with comments addressed. Thanks!

Yes, I will fix this. I'll try to get done on Monday - been working on some other things today.

peixin added inline comments.May 17 2022, 12:29 AM
flang/lib/Lower/OpenMP.cpp
408

Can you add this back, rebase and land this patch?

@kiranchandramohan @Leporacanthicus Can we promote the priority of the upstream work? I compared fir-dev and LLVM main. We have 5 patches not upstreamed for OpenMP: this one, D125465, D125740, https://github.com/flang-compiler/f18-llvm-project/pull/1559 (@kiranchandramohan Can you also upstream this?), schedule chunk/modifier (@Leporacanthicus Can you upstream it?) For this one (https://github.com/flang-compiler/f18-llvm-project/pull/1178), it is not in a hurry to upstream it since it also has some works in FIR and it won't affect the following refactor work.

The current framework in OpenMP.cpp does not organize the clauses very well. Continuing adding the support for other constructs can easily bring in some bugs. OpenMP Loop constructs (https://github.com/llvm/llvm-project/blob/5646d82885f64f1f886d84aff5422697f355f6c4/flang/lib/Parser/openmp-parsers.cpp#L261-L294) and Block constructs do need to be refactored. After the above upstream work finishes, @shraiysh proposes one good patch (D121583) to reorganize the clauses.

All in all, the more other additional code merged, the harder the refactor work will be.

flang/lib/Lower/OpenMP.cpp
408

We were originally planning to push forward with upstreaming this week. Unfortunately, something else came up. @Leporacanthicus is on leave today.
I will get to the remaining upstreaming and patch reviews from Thursday. Apologies for the delay.

peixin added inline comments.May 17 2022, 1:53 AM
flang/lib/Lower/OpenMP.cpp
408

OK. Thanks.

Replace if with assert, as it was in earlier code (before the loop was added)

Add assert for bounds back in, remove if-statment. This puts the code in
the new loop back to how the code was before the loop.

flang/lib/Lower/OpenMP.cpp
408

Maybe graceful error out during semantic checks for this (not necessarily in this patch) plus an assert here should be added.

There is some checking in the Sema that produces an error when you try to do this:

i = 0
!$OMP DO
do
   if (i .eq. 100) exit
   i = i + 1
end do
!$OMP END DO

With other errors if you use do while (...) [Although when I did both, it only showed errors for the latter - I'm guessing because it does some random order evaluation and checking and quits on first error... Either way, we should not hit this assert. I have added one - and removed the if-statement, since it's redundant [and if we crash on bounds being NULL in non-debug, it's probably better than some kind weirder code-generation failure or bug somewhere else]

This revision was automatically updated to reflect the committed changes.