Page MenuHomePhabricator

[flang][openacc] Lower clauses on loop construct to OpenACC dialect
ClosedPublic

Authored by clementval on Sep 9 2020, 9:00 AM.

Details

Summary

Lower OpenACCLoopConstruct and most of the clauses to the OpenACC acc.loop operation in MLIR.
This patch refelcts what can be upstream from PR flang-compiler/f18-llvm-project#419

Diff Detail

Event Timeline

clementval created this revision.Sep 9 2020, 9:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 9:00 AM
clementval requested review of this revision.Sep 9 2020, 9:00 AM
teemperor requested changes to this revision.Sep 9 2020, 10:28 AM
teemperor added a subscriber: teemperor.
teemperor added inline comments.
lldb/tools/lldb-vscode/VSCode.cpp
41 ↗(On Diff #290751)

Unrelated reformatting?

This revision now requires changes to proceed.Sep 9 2020, 10:28 AM
clementval added inline comments.Sep 9 2020, 10:48 AM
lldb/tools/lldb-vscode/VSCode.cpp
41 ↗(On Diff #290751)

Sure it shouldn't be here. I'll remove it.

SouraVX edited projects, added Restricted Project; removed Restricted Project.Sep 9 2020, 10:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 10:48 AM

Since bbc based test can't be added here, would you mind adding a unit-test here ?

Remove unrelated changes

Herald added a project: Restricted Project. · View Herald TranscriptSep 9 2020, 10:51 AM
teemperor resigned from this revision.Sep 9 2020, 10:51 AM

Thanks!

Since bbc based test can't be added here, would you mind adding a unit-test here ?

Well I have to see how to add a meaningful unit test here. Lowering unit tests that are currently upstreamed seem to just copy-paste the lowering code which does not really test the lowering code itself.

Address clang-tiday warning

SouraVX accepted this revision.Sep 9 2020, 11:26 PM

Since bbc based test can't be added here, would you mind adding a unit-test here ?

Well I have to see how to add a meaningful unit test here. Lowering unit tests that are currently upstreamed seem to just copy-paste the lowering code which does not really test the lowering code itself.

Yes those are duplicate in a sense, but they are still tests(since nothing else is there) and can self validate well-formedness of the operation(which is one of the major pre-requisite for proceeding further with Lowering).
+ They will ensure matches with community criteria. up-streaming in testable chunks.

I do not have any strong reservations :). Feel free to merge, if you don't like the idea in general :)

This revision is now accepted and ready to land.Sep 9 2020, 11:26 PM
SouraVX removed a project: Restricted Project.Sep 9 2020, 11:28 PM

Yes those are duplicate in a sense, but they are still tests(since nothing else is there) and can self validate well-formedness of the operation(which is one of the major pre-requisite for proceeding further with Lowering).
+ They will ensure matches with community criteria. up-streaming in testable chunks.

My principal concern is that they do not test the code in genACC or genOMP so they won't catch any problem or regression there. I'm trying to put in place a unit test that make use of the genACC function directly. If that works I'll add the test before landing the patch.

SouraVX added a comment.EditedSep 10 2020, 7:22 AM

Yes those are duplicate in a sense, but they are still tests(since nothing else is there) and can self validate well-formedness of the operation(which is one of the major pre-requisite for proceeding further with Lowering).
+ They will ensure matches with community criteria. up-streaming in testable chunks.

My principal concern is that they do not test the code in genACC or genOMP so they won't catch any problem or regression there.

I think that is justified and already tested with full coverage in regression(end-to-end) test with bbc.

I'm trying to put in place a unit test that make use of the genACC function directly. If that works I'll add the test before landing the patch.

+1
The reason why I'm saying this that we(you) also don't want both branches going out-of-sync and regressing further development :)