This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP] Add implementation of privatisation
ClosedPublic

Authored by kiranchandramohan on Mar 28 2022, 9:51 AM.

Details

Summary

Privatisation creates local copies of variables in the OpenMP region.
Two functions createHostAssociateVarClone and copyHostAssociateVar
are added to create a clone of the variable for basic privatisation and to
copy the contents for first-privatisation.

Note: Tests for more data-types will be added when the fir.do_loop is
upstreamed.

This is part of the upstreaming effort from the fir-dev branch in [1].
[1] https://github.com/flang-compiler/f18-llvm-project

Co-authored-by: Jean Perier <jperier@nvidia.com>
Co-authored-by: Eric Schweitz <eschweitz@nvidia.com>
Co-authored-by: Peter Klausler <pklausler@nvidia.com>
Co-authored-by: Valentin Clement <clementval@gmail.com>
Co-authored-by: Sourabh Singh Tomar <SourabhSingh.Tomar@amd.com>
Co-authored-by: Nimish Mishra <neelam.nimish@gmail.com>
Co-authored-by: Peixin-Qiao <qiaopeixin@huawei.com>

Diff Detail

Event Timeline

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 28 2022, 9:51 AM
kiranchandramohan requested review of this revision.Mar 28 2022, 9:51 AM
peixin accepted this revision.Mar 28 2022, 6:12 PM
This revision is now accepted and ready to land.Mar 28 2022, 6:12 PM
shraiysh added inline comments.Mar 28 2022, 11:30 PM
mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
30 ↗(On Diff #418625)

Nit: This looks unrelated. (I think a rebase would clear this, as this was added)

kiranchandramohan edited the summary of this revision. (Show Details)Mar 29 2022, 3:53 AM
NimishMishra accepted this revision.Mar 30 2022, 4:57 AM

LGTM. Thanks for this.

flang/test/Lower/OpenMP/omp-parallel-private-clause.f90
5

Nit: A small question: would we be soon retiring bbc? Does it make sense to use flang-new for all upstream patches now?

I will check the windows test failure.

flang/test/Lower/OpenMP/omp-parallel-private-clause.f90
5

I am not in a position to answer this question. It is primarily for the FIR team to decide on the future of bbc. Till a decision is made, I think it will be best to use both flang-new and bbc for existing tests that are being upstreamed. For new OpenMP tests you may use flang-new.

I will update this test to run with flang-new as well.

mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp
30 ↗(On Diff #418625)

OK. Rebased to fix.

There was a link failure since it was failing to find the getAllocaBlock function which is part of the interface.

peixin added inline comments.Apr 2 2022, 2:29 AM
flang/test/Lower/OpenMP/omp-parallel-private-clause.f90
5

They are discussing flang-new and bbc. Most upstream patches use bbc to test FIR genrated from PFT currently. So I think it's better we keep the same. If it needs to be fixed in future, it will be fixed together with FIR tests.

Considering this, can you change the current flang_fc1 -emit-fir -fopenmp back to bbc -emit-fir -fopenmp for current tests (sections.f90, flush.f90, master.f90, critical.f90)?

43

Can you add these test cases here since this patch has the corresponding function code which is not tested?

peixin added inline comments.Apr 2 2022, 2:32 AM
flang/test/Lower/OpenMP/omp-parallel-private-clause.f90
5

Sorry for forgetting put the link https://reviews.llvm.org/D121171#3370137.

flang/test/Lower/OpenMP/omp-parallel-private-clause.f90
5

@peixin This was discussed in https://reviews.llvm.org/D121118 and it seems the decision was to use both tools to run the OpenMP tests for now.

As far as OpenMP is concerned we only need a mechanism to run the tests and we can use both tools to test. The FIR team might have specific use cases and it might make sense to test FIR generation with bbc. But we do not have such use-cases.

Also, a lot of the intrinsics tests use both tools to test.

43

Will add.

peixin added inline comments.Apr 4 2022, 4:11 AM
flang/test/Lower/OpenMP/omp-parallel-private-clause.f90
5

Got it. Thanks.

@Meinersbur The windows bot is failing for this patch since 128-bit integer support is not there. Is there anything in Windows to enable __SIZEOF_INT128__. See relevant code in link below.
https://github.com/llvm/llvm-project/blob/main/flang/lib/Lower/IO.cpp#L422

Disable tests on Windows. Use tests from @peixin.

kiranchandramohan edited the summary of this revision. (Show Details)Apr 8 2022, 9:19 AM
peixin added a comment.Apr 8 2022, 9:48 AM

This needs rebase.

Rebase as suggested by @peixin.

peixin accepted this revision.Apr 9 2022, 8:19 PM

LGTM again.

flang/include/flang/Lower/AbstractConverter.h
95

Nit: give -> given?

flang/include/flang/Lower/AbstractConverter.h
95

Fixed while committing. Thanks.