This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Support for Collapse

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



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.


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


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


Nit: I prefer the previous formatting.


Nit: 2->3

LGTM except for @kiranchandramohan 's comments.




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.


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?


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.


nit: end do?

peixin added inline comments.May 11 2022, 6:49 AM

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


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


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



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.


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!


+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

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, (@kiranchandramohan Can you also upstream this?), schedule chunk/modifier (@Leporacanthicus Can you upstream it?) For this one (, 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 ( 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.


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

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.


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
   if (i .eq. 100) exit
   i = i + 1
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.