Page MenuHomePhabricator

[flang][openacc] Lower init and shutdown directive
ClosedPublic

Authored by clementval on Oct 30 2020, 11:34 AM.

Details

Summary

This patch upstream the lowering of Init and Shutdown directives that was initially done in
https://github.com/flang-compiler/f18-llvm-project/pull/529

Diff Detail

Event Timeline

clementval created this revision.Oct 30 2020, 11:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2020, 11:34 AM
clementval requested review of this revision.Oct 30 2020, 11:34 AM
clementval added a project: Restricted Project.Oct 30 2020, 11:48 AM
schweitz accepted this revision.Wed, Nov 4, 10:08 AM
This revision is now accepted and ready to land.Wed, Nov 4, 10:08 AM
This revision was automatically updated to reflect the committed changes.

Do you have tests for this?

Do you have tests for this?

Test still live in the fir-dev branch. Since not all the infra is present for the test to be executed, there were not upstreamed yet. the rest of the fir-dev branch should be upstream very soon but I can hardly influence that.

Do you have tests for this?

Test still live in the fir-dev branch. Since not all the infra is present for the test to be executed, there were not upstreamed yet. the rest of the fir-dev branch should be upstream very soon but I can hardly influence that.

I'd like to see IR -> IR unit-test (mlir-opt style) provided here. I don't think it is reasonable to upstream code without proper testing.

Do you have tests for this?

Test still live in the fir-dev branch. Since not all the infra is present for the test to be executed, there were not upstreamed yet. the rest of the fir-dev branch should be upstream very soon but I can hardly influence that.

I'd like to see IR -> IR unit-test (mlir-opt style) provided here. I don't think it is reasonable to upstream code without proper testing.

There are IR to IR test for those operations in mlir itself where the dialect is located.

All OpenMP and OpenACC lowering are waiting on the fir-dev to be upstream.

So should we un-upstream the code for the time being?

All OpenMP and OpenACC lowering are waiting on the fir-dev to be upstream.

Is this "dead code" right now then?

So should we un-upstream the code for the time being?

If this can't be tested / is dead code, I'm not super comfortable having this in-tree. What is the plan for fir-dev development to move upstream, I'm still quite puzzled and don't understand why the development does not happen in tree, I think I asked on the mailing list but never got an answer.

clementval added a comment.EditedTue, Nov 24, 2:57 PM

If this can't be tested / is dead code, I'm not super comfortable having this in-tree. What is the plan for fir-dev development to move upstream, I'm still quite puzzled and don't understand why the development does not happen in tree, I think I asked on the mailing list but never got an answer.

There was discussion on the last few calls to move it upstream very soon. A RFC should have been sent to coordinate the upstreaming of the fir-dev patch(es). Looks like it hasn't moved yet. @kiranchandramohan @SouraVX @schweitz @sscalpone Do you know if there is any progress with the upstreaming process?

@mehdi_amini I'm fine with removing this code can be upstream again later if you prefer. Same with all the code in flang/lib/Lower/OpenACC.cpp.

The tests have been created and they are ready to be upstreamed as soon as I can (https://github.com/flang-compiler/f18-llvm-project/tree/fir-dev/flang/test/Lower/OpenACC)

I'm fine with leaving it there for now, looking forward to see everything upstream!

I'm fine with leaving it there for now, looking forward to see everything upstream!

Thanks! Agreed it isn't ideal for the moment.

There was discussion on the last few calls to move it upstream very soon. A RFC should have been sent to coordinate the upstreaming of the fir-dev patch(es). Looks like it hasn't moved yet. @kiranchandramohan @SouraVX @schweitz @sscalpone Do you know if there is any progress with the upstreaming process?

AFAIK(based on @jdoerfert comment in our last meet) it's up for internal review for Lab folks.