Page MenuHomePhabricator

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

Authored by sameeranjoshi on Sat, Oct 31, 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.Sat, Oct 31, 12:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Oct 31, 12:15 PM
sameeranjoshi requested review of this revision.Sat, Oct 31, 12:15 PM
clementval added inline comments.Sat, Oct 31, 12:19 PM
flang/lib/Semantics/resolve-directives.cpp
357

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

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

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.Sat, Oct 31, 12:36 PM

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

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

Why does aligned require a new symbol?

974

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.Mon, Nov 2, 10:57 AM
sameeranjoshi added inline comments.
flang/lib/Semantics/resolve-directives.cpp
323

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

I have removed it.

974

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

Two more questions.

flang/lib/Semantics/resolve-directives.cpp
285–306

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;
}
301

Can this be false here?

sameeranjoshi marked 2 inline comments as done.

Address review comments.

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

Thanks for reviewing.

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

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.Tue, Nov 3, 5:43 AM
kiranchandramohan added inline comments.
flang/lib/Semantics/resolve-directives.cpp
301
This revision is now accepted and ready to land.Tue, Nov 3, 5:43 AM
clementval added inline comments.Tue, Nov 3, 8:17 AM
flang/lib/Semantics/resolve-directives.cpp
301

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.Wed, Nov 4, 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.