Page MenuHomePhabricator

[flang][openacc] Add missing loop construct restriction and validity tests
ClosedPublic

Authored by clementval on Dec 2 2020, 7:34 PM.

Details

Summary

Add restriction on loop construct associated with DO CONCURRENT. Add couple of tests to ensure
clause validity checks.

Diff Detail

Event Timeline

clementval created this revision.Dec 2 2020, 7:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2020, 7:34 PM
clementval requested review of this revision.Dec 2 2020, 7:34 PM

The tests look good, just a few comments/queries?
I am really not familiar with canonicalization, but I could grasp what's defined in this patch.

flang/lib/Semantics/canonicalize-acc.cpp
68

If that's a restriction why is this not in the form of a semantic error message?

87

Which standard ?
There are patches for 3.1 up gradation, will line numbers change in that case?
Is 3.0 assumed when no reference?

99

A C++ beginners query -
Why use std::get_if over std::holds_alternative if we are not utilizing the return pointer?

102

nit:Was that or instead of and ?

clementval marked 2 inline comments as done.Dec 4 2020, 10:04 AM
clementval added inline comments.
flang/lib/Semantics/canonicalize-acc.cpp
68

The error message is issued in CheckDoConcurrentClauseRestriction in case there is a tile clause associated. Here we just bypass the logic and we do not issue the message a second time.

87

From no one all reference are from 3.1. Right now it's like a transition phase until patch D92120 is approved and landed.

99

std::holds_alternative would be fine in this case. I'll update the patch. Thx.

102

I just use the text from the standard. or would work as well.

clementval marked 2 inline comments as done.Dec 4 2020, 12:28 PM
clementval added inline comments.
flang/lib/Semantics/canonicalize-acc.cpp
87

From now on ... and not "no one"!

clementval marked 2 inline comments as done.

Address review comments + rebase

sameeranjoshi accepted this revision.Dec 8 2020, 7:10 AM

Thanks for clarifications.
Approving it just take care of the build issue before merging.
LGTM.

flang/lib/Semantics/canonicalize-acc.cpp
99

Please check syntax this doesn't build.
no &.

This revision is now accepted and ready to land.Dec 8 2020, 7:10 AM
clementval updated this revision to Diff 310221.Dec 8 2020, 8:05 AM

Rebase + fix build problem

clementval marked an inline comment as done.Dec 8 2020, 8:05 AM
clementval added inline comments.
flang/lib/Semantics/canonicalize-acc.cpp
99

Thanks for catching this. Don't know what I did with this :-)

This revision was landed with ongoing or failed builds.Dec 8 2020, 11:12 AM
This revision was automatically updated to reflect the committed changes.
clementval marked an inline comment as done.