Page MenuHomePhabricator

[Flang][openmp]Fix crash in OpenMP semantic check( bug 48308)
ClosedPublic

Authored by sameeranjoshi on Dec 4 2020, 12:06 AM.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Dec 4 2020, 12:06 AM
sameeranjoshi requested review of this revision.Dec 4 2020, 12:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: sstefan1. · View Herald Transcript

The title doesn't match the fix. You are fixing smth in name resolution and not semantic checking.

tskeith added inline comments.Dec 4 2020, 8:54 AM
flang/test/Semantics/bug48308.f90
1 ↗(On Diff #309470)

I don't like the name of this test. The name should indicate what functionality it is testing, like every other test.

sameeranjoshi added inline comments.Dec 4 2020, 9:44 AM
flang/test/Semantics/bug48308.f90
1 ↗(On Diff #309470)

I was thinking similar to the way GNU compiler captures them (pr48308.f90) so as to find the test quickly from the name on bugzilla.
Will add a better name.

change test name.
Change title.

clementval added inline comments.Dec 10 2020, 12:34 PM
flang/lib/Semantics/resolve-directives.cpp
878

Not specially for this patch but just a quick question: What is the ultimate standard we are targeting in Flang? 4.5 or 5.0. I have seen both in new patches and since there is no way to enforce one standard or another it feel weird to mix them. Maybe I missed this discussion but if you have any information it would be great for my personal knowledge.

883

First line reference 4.5 and here 5.0 .... I think it would be nice to have a homogeneity here.

flang/lib/Semantics/resolve-directives.cpp
878

Arm's immediate requirement is to replace the existing Flang (flang-compiler/flang) compiler with llvm/flang. The current compiler has partial support for 4.5 and no support for 5.0. Hence you see commits from our side referring to 4.5. Once we are done with checks for 4.5 we will do an upgrade to 5.0. AMD and others will continue to work on 5.0.

I agree that there is weirdness in this. But I anticipate quick progress on semantic checks and this will go away in a few months time.

883

I think specific point might not be from the OpenMP standard. It is probably a general comment that do while loops might not have iteration variables.

But the ones below are forward looking comments.

clementval added inline comments.Dec 10 2020, 6:56 PM
flang/lib/Semantics/resolve-directives.cpp
878

Thanks for the clarification @kiranchandramohan. Is this written somewhere in the doc? If not, it would maybe be nice to have just a line or two in the documentation just mentioning this "transition" phase.

883

Ok. Just very confusing to have 3 versions of the standard on the same comment blocks.

897

Are these tabs?

kiranktp added inline comments.Dec 11 2020, 3:19 AM
flang/lib/Semantics/resolve-directives.cpp
883

As Valentine mentioned, having different standards mentioned under one construct will be very confusing. It would be good to remove references to 5.0 and 5.1 here. As and when we start migrating the next OpenMP version, we can modify these appropriately.

kiranktp added inline comments.Dec 11 2020, 3:24 AM
flang/lib/Semantics/resolve-directives.cpp
878

This is pending for quite sometime. We should add OpenMP 5.0 grammar file as well.
Would it make sense to modify "OpenMP 4.5 Grammar" page to "OpenMP Grammar" and add details about current implementation and future transition along with both grammar[4.5 and 5.0]?

sameeranjoshi added inline comments.Dec 14 2020, 11:06 AM
flang/lib/Semantics/resolve-directives.cpp
883

I think specific point might not be from the OpenMP standard. It is probably a general comment that do while loops might not have iteration variables.

But the ones below are forward looking comments.

Yes this is correct.
Indeed it would have been confusing when actually changing something related to code in that part, if I wouldn't have mentioned references as
Until 5.0 usage of loops apart from do loop is not well specified.
In 5.1 there are provisions for one of the forms of loops(do concurrent).
And the reference for comment related to function was already present but inorder to find 2.15.1.1 you needed to explicitly mention the 4.5 standard as there are many references to standard.

Try to simplify comments.

This revision is now accepted and ready to land.Dec 16 2020, 5:29 AM
clementval accepted this revision.Dec 16 2020, 6:45 AM

Thanks for updating the comment. It's clearer now