This is an archive of the discontinued LLVM Phabricator instance.

[flang][OpenMP] Added allocate clause translation for OpenMP block constructs
ClosedPublic

Authored by shraiysh on Mar 25 2022, 7:26 AM.

Details

Summary

This patch adds translation for allocate clause for parallel and single
constructs.

Also added tests for block constructs.

This patch also adds tests for parallel construct which were not added earlier.

Co-authored-by: Sourabh Singh Tomar <SourabhSingh.Tomar@amd.com>

Diff Detail

Event Timeline

shraiysh created this revision.Mar 25 2022, 7:26 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
shraiysh requested review of this revision.Mar 25 2022, 7:26 AM
shraiysh updated this revision to Diff 418262.Mar 25 2022, 9:56 AM

Handle pre-merge check failure.

shraiysh edited the summary of this revision. (Show Details)Mar 25 2022, 9:58 AM
This comment was removed by shraiysh.
NimishMishra accepted this revision.Mar 30 2022, 7:18 AM

LGTM. Thanks for this. A couple of small nits, please let me know what you think.

flang/lib/Lower/OpenMP.cpp
192

Nit: ProcBindKind has 6 possibilities in total. Although I strongly believe checks would be there, but can you confirm if semantic checks are in place that guard against the other three (Primary, Default, and Unknown) from trickling down till here?

flang/test/Lower/OpenMP/parallel.f90
67

Nit: You also add tests for num_threads, if, proc_bind. Can you edit the patch description to this effect? I think that would make it complete, since we aren't just looking at tests related to allocate clause here.

This revision is now accepted and ready to land.Mar 30 2022, 7:18 AM
shraiysh edited the summary of this revision. (Show Details)Mar 31 2022, 11:44 AM
shraiysh updated this revision to Diff 419547.Mar 31 2022, 12:46 PM

Addressed comments.

shraiysh marked 2 inline comments as done.Mar 31 2022, 10:27 PM

I will merge this tomorrow if there are no further comments.

peixin added a comment.Apr 1 2022, 1:06 AM

One more nit for the title: Added allocate clause translation for block constructs -> Added allocate clause translation for OpenMP block constructs since fortran also has one block construct.

Sorry for the late review.

flang/lib/Lower/OpenMP.cpp
188

This if clause comment should be in line192.

195

This num_threads clause comment should be in 200. Or don't need it since the code is prettry clear.

233

Add one else branch with hard TODO for these clauses.

242

This comment should be in line 243 or no need it. I think the code is prettry clear.

251–252

Remove lines 255 and 256. The same for others.

peixin added inline comments.Apr 2 2022, 2:41 AM
flang/test/Lower/OpenMP/parallel.f90
3

The same for flang_fc1 -emit-fir -fopenmp.

14

Nit: Please add CHECK-LABEL for each subroutine.

shraiysh retitled this revision from [flang][OpenMP] Added allocate clause translation for block constructs to [flang][OpenMP] Added allocate clause translation for OpenMP block constructs.Apr 7 2022, 12:20 PM
shraiysh updated this revision to Diff 421417.Apr 7 2022, 10:23 PM
shraiysh marked 6 inline comments as done.

Addressed comments.

shraiysh added inline comments.Apr 7 2022, 10:44 PM
flang/test/Lower/OpenMP/parallel.f90
3

I didn't understand this. Can you please elaborate it?

peixin accepted this revision.Apr 8 2022, 12:33 AM
peixin added inline comments.
flang/test/Lower/OpenMP/parallel.f90
3

Please forget about this. Kiran has explained to me. There is no much difference using flang-new or bbc for us to test openmp code.

shraiysh marked 2 inline comments as done.Apr 8 2022, 2:58 AM
shraiysh added inline comments.
flang/test/Lower/OpenMP/parallel.f90
3

Okay, thank you. I will land this.

This revision was automatically updated to reflect the committed changes.
shraiysh marked an inline comment as done.