Page MenuHomePhabricator

[Flang][OpenMP] Fix 'Internal: no symbol found' for OpenMP aligned and linear clause.
ClosedPublic

Authored by sameeranjoshi on Oct 31 2020, 12:15 PM.

Details

Summary

The initial approach was to go with changing parser nodes from std::list<parser::Name> to OmpObjectList, but that might have lead to illegal programs.
Resolving the symbols inside OmpAttributeVisitor.
Fix a couple of XFAIL tests.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Oct 31 2020, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 31 2020, 12:15 PM
sameeranjoshi requested review of this revision.Oct 31 2020, 12:15 PM
clementval added inline comments.Oct 31 2020, 12:19 PM
flang/lib/Semantics/resolve-directives.cpp
343

So in the end you didn't need resolve-names and an easy resolve name inside resolve directive was doable...

sameeranjoshi added inline comments.Oct 31 2020, 12:26 PM
flang/lib/Semantics/resolve-directives.cpp
343

Yes :), hopefully when I debugged how the actual resolution occurs.
But in later patches we might end up with needless repetition of code for fortran and OMP/OACC.

sameeranjoshi added a project: Restricted Project.Oct 31 2020, 12:36 PM

Thanks @sameeranjoshi for this patch. Couple of questions inline.

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

Why does aligned require a new symbol?

957

Should we call AddToContextObjectWithDSA only for those flags which correspond to DSA (default, shared, private, lastprivate, firstprivate, linear)?

Address review comments.

sameeranjoshi marked 3 inline comments as done.Nov 2 2020, 10:57 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/resolve-directives.cpp
312

Looks like I was in assumption that we need the same flags from flang/include/flang/Semantics/symbol.h.

I have removed it.

957

Thanks for making me aware of that.
I have added a guard.

Two more questions.

flang/lib/Semantics/resolve-directives.cpp
272–295

Would one of the following work and would it be better?

->

bool Pre(const parser::OmpLinearClause::WithoutModifier &x) {
  ResolveOmpNameList(x.names, Symbol::Flag::OmpLinear);
  return false;
}
bool Pre(const parser::OmpLinearClause::WithModifier &x) {
  ResolveOmpNameList(x.names, Symbol::Flag::OmpLinear);
  return false;
}

->

bool Pre(const parser::OmpLinearClause &x) {
  std::visit(common::visitors{
                 [&](const parser::OmpLinearClause::WithoutModifier
                         &linearWithoutModifier) {
                   ResolveOmpNameList(linearWithoutModifier.names, Symbol::Flag::OmpLinear);
                 },
                 [&](const parser::OmpLinearClause::WithModifier
                         &linearWithModifier) {
                   ResolveOmpNameList(linearWithModifier.names, Symbol::Flag::OmpLinear);

                 },
             },
      x.u);
  return false;
}
288

Can this be false here?

sameeranjoshi marked 2 inline comments as done.

Address review comments.

sameeranjoshi marked 2 inline comments as done.Nov 2 2020, 10:08 PM

Thanks for reviewing.

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

Keeping it for consistency sake with the convention followed in the file.
Just asking if you know is there some reason to use some specific boolean value?

kiranchandramohan accepted this revision.Nov 3 2020, 5:43 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/resolve-directives.cpp
288
This revision is now accepted and ready to land.Nov 3 2020, 5:43 AM
clementval added inline comments.Nov 3 2020, 8:17 AM
flang/lib/Semantics/resolve-directives.cpp
288

If you raise an error here you might want to return false to not go further in the visit since you might not have the information to continue in the nested nodes.

sameeranjoshi marked an inline comment as done.Nov 4 2020, 5:44 AM

Thank you for detailed information.
I missed a test which I realized when I was trying to rebase on master.
https://github.com/llvm/llvm-project/blob/f1a96de1bc8db527b5eb820c36c17e275900ca2b/flang/test/Semantics/omp-declarative-directive.f90#L14

I debugged to find that the aligned node is not visited for the above test, is !$omp declare missing here to visit ?

I debugged to find that the aligned node is not visited for the above test, is !$omp declare missing here to visit ?

Looks like declarative construct are visited from here in the OpenMP name resolution:
https://github.com/llvm/llvm-project/blob/f1a96de1bc8db527b5eb820c36c17e275900ca2b/flang/lib/Semantics/resolve-directives.cpp#L225

After the patch D89385 there were regressions in this review before merging
it, probably due to bool Pre(const parser::SpecificationPart &x) was made to
return true hence the nodes under it were visited.
And the tests consisted of one such case which was causing fatal internal error: CHECK(!ompContext_.empty()).
Resolve symbols which were not found in OpenMPDeclareSimdConstruct to make this patch working.