This is an archive of the discontinued LLVM Phabricator instance.

[Flang][OpenMP] Rework parser changes for OpenMP atomic construct.
ClosedPublic

Authored by sameeranjoshi on Oct 7 2020, 6:36 AM.

Details

Summary

OmpStructureChecker is supposed to work only with parser::OmpClause
after tablegen changes for OpenMP and OpenACC were introduced.
Hence OmpMemoryOrderClause, OmpAtomicMemoryOrderClause and similar ones were failing
to catch semantic errors, inspite of having code for semantic checks.
This patch tries to change parser for OmpMemoryOrderClause and similar dependent ones
and use OmpClauseList which resides/comes from common tablegen for OpenMP/OpenACC eventually using parser::OmpClause.

This patch also tries to :

  1. Change OmpCriticalDirective in openmp-parsers.cpp to support OmpClauseList.
  2. Check-flang regresses when changes were introduced due to missing semantic checks in OmpCritical, patch implements them at the minimal level to pass the regression.
  3. Change tablegen to support Hint clause.
  4. Adds missing source locations CharBlock Source in each atomic construct.
  5. Remove dead code realted to memory-order-clauses after moving to OmpClauseList.

Diff Detail

Event Timeline

sameeranjoshi created this revision.Oct 7 2020, 6:36 AM
Herald added a project: Restricted Project. · View Herald Transcript
sameeranjoshi requested review of this revision.Oct 7 2020, 6:36 AM
sameeranjoshi added a project: Restricted Project.Oct 7 2020, 6:37 AM

Just to make things clear. The OmpStructureChecker can work with other specific clauses struct you introduce but of course if you want to leverage the current code that is in you have to use OmpClause. But in the Enter/Leave function you can perform any semantic check you want. See for example device_type Sema in OpenACC. It is specific so of course it doesn't fall into the TableGen automatic checks. So this is fine to make the change you did but you could have also implement these checks with specific code for OmpAtomicMemoryOrderClause. Anyway I think if the atomic construct fits with OmpClause this is a better approach since the implementation is used by all other clauses.

Just added small comment. The change seems fine to me but I didn't check if it is conform to the OpenMP spec.

flang/include/flang/Parser/parse-tree.h
3598

Can your remove the optional? Empty list should do the work ...

flang/lib/Parser/openmp-parsers.cpp
219

Remove new line.

Review comments addressed.

A few minor/nit comments. Could you add a few unparse tests in the test/Parser directory?

flang/include/flang/Parser/parse-tree.h
3598

Why is it an OmpClauseList and not OmpClause? It is just a hint clause right? So should this be std::optional<OmpClause>?

flang/lib/Parser/openmp-parsers.cpp
162

Nit: empty line?

214

Thanks for moving this to the correct place.

224

Nit: Empty Line

  1. Revert OmpClauseList back to std::optional<OmpClause>.

Sorry a copy paste typo.

  1. Add unparse tests.

Why do we need unparser tests here, as unparsing would be removed at some point
in time? So should the tests be removed at that time.

clementval added a comment.EditedOct 8 2020, 6:41 AM
  1. Add unparse tests.

Why do we need unparser tests here, as unparsing would be removed at some point
in time? So should the tests be removed at that time.

Who said the unparse features will be removed?

kiranchandramohan accepted this revision.Oct 8 2020, 1:40 PM

OmpStructureChecker is supposed to work only with parser::OmpClause
after tablegen changes for OpenMP and OpenACC were introduced.

I think this was not related to tablegen changes. The semantic checks
only worked with OmpClauses.

Why do we need unparser tests here, as unparsing would be removed at some point
in time? So should the tests be removed at that time.

Sorry, I was not aware that they are going to be removed. I guess we can keep it till then.
I would prefer keeping the unparsing since it provides a mechanism to 1) test the parser
and 2) do source to source translation.

BTW, are allowed clauses and all handled automatically or will they require a separate patch?

This revision is now accepted and ready to land.Oct 8 2020, 1:40 PM

I would prefer keeping the unparsing since it provides a mechanism to 1) test the parser
and 2) do source to source translation.

+1 I thing the source-to-source translation will interest many people.

Sorry, I was not aware that they are going to be removed. I guess we can keep it till then.
I would prefer keeping the unparsing since it provides a mechanism to 1) test the parser
and 2) do source to source translation.

Those are my personal understandings from how current f18 behaves by unparsing and passing it to an external compiler specified by F18_FC , I am too unaware if unparsing would be removed officially, but if projects which might interest people are possible it's ideal to retain that.

BTW, are allowed clauses and all handled automatically or will they require a separate patch?

I think semantic checks for atomic needs separate patch which might need to upgrade to OpenMP 5.0 changes.

SouraVX added inline comments.Oct 9 2020, 5:29 AM
flang/lib/Parser/unparse.cpp
2267

NIT: Can't we use the type name here ? get<0> does the job but makes the code less readable.

sameeranjoshi marked an inline comment as done.Oct 9 2020, 6:14 AM
sameeranjoshi added inline comments.
flang/lib/Parser/unparse.cpp
2267

If you take a look at how parse-tree.h represents AtomicUpdate

// ATOMIC UPDATE
struct OmpAtomicUpdate {
  TUPLE_CLASS_BOILERPLATE(OmpAtomicUpdate);
  std::tuple<OmpClauseList, Verbatim,
      OmpClauseList, Statement<AssignmentStmt>,
      std::optional<OmpEndAtomic>>
      t;
};

When std::tuple contains more than 1 repeated types, there is ambiguity for compiler to choose the type.
This makes us use indices to make it explicit.

BTW, are allowed clauses and all handled automatically or will they require a separate patch?

I think semantic checks for atomic needs separate patch which might need to upgrade to OpenMP 5.0 changes.

If semantic checks for 4.5 clauses don't automatically work then there is a regression since previously the parser correctly only allowed MemoryOrder Clauses. Now that we have generalized to OmpClause for Atomic, it might be better to add tests for semantic checks in this patch itself.

SouraVX added inline comments.Oct 9 2020, 9:24 AM
flang/lib/Parser/unparse.cpp
2267

Gotcha Thanks! Maybe get<0/*OmpClauseList*/>. No hard reservations, This may look not so good(Personal taste). Feel free to ignore otherwise :)

sameeranjoshi marked an inline comment as done.EditedOct 12 2020, 11:43 PM

BTW, are allowed clauses and all handled automatically or will they require a separate patch?

I think semantic checks for atomic needs separate patch which might need to upgrade to OpenMP 5.0 changes.

If semantic checks for 4.5 clauses don't automatically work then there is a regression since previously the parser correctly only allowed MemoryOrder Clauses. Now that we have generalized to OmpClause for Atomic, it might be better to add tests for semantic checks in this patch itself.

There are no semantic checks for even 4.5 as well nor any tests.
It needs more work which I think should be better to come with a new patch specific to semantic checks.
Until then is that ok to merge this, as there is a dependency for semantics with this patch.

@sameeranjoshi Is it incorrect to assume that the allowed clauses will work because it is handled by tablegen? Like for e.g. will num_threads be accepted as a clause with atomic?
https://github.com/llvm/llvm-project/blob/1687a8d83b702332c4aae4f2a95d27c16688418d/llvm/include/llvm/Frontend/OpenMP/OMP.td#L444

What I understand from your query is, you are basically trying to ask if we are using OmpClauseList in parser-tree.h so should all the clauses be allowed on atomic construct, which is not expected and you are trying to find where are the restrictions on the various clauses from OmpClauseList on Atomic construct.

@sameeranjoshi Is it incorrect to assume that the allowed clauses will work because it is handled by tablegen?

IIUC, it's partially correct.
The allowedClauses which are currently present in OMP.td will work in catching the test cases which don't obey what OMP.td has defined.
e.g for test cases with num_threads clause, which is not found in allowedClauses of def OMP_Atomic is caught in parsing stage.

!!$omp atomic num_threads read
    i = j

Above test is a syntax error.

Now coming to what standard 5.0 says for one of the constraints:

At most one memory-order-clause may appear on the construct.

So a test like

!$omp atomic SEQ_CST SEQ_CST read
     i  = j

Is currently not flagged as semantic error.
The possible reason which Valentine explained me was that "there should be at least a Enter function for each clauses with a CheckAllowed inside check-omp-structure.cpp".
And those functions lack in check-omp-structure.cpp which was my next patch.

Like for e.g. will num_threads be accepted as a clause with atomic?

no.

https://github.com/llvm/llvm-project/blob/1687a8d83b702332c4aae4f2a95d27c16688418d/llvm/include/llvm/Frontend/OpenMP/OMP.td#L444

please correct me If I am still missing something.

IIUC, it's partially correct.
The allowedClauses which are currently present in OMP.td will work in catching the test cases which don't obey what OMP.td has defined.
e.g for test cases with num_threads clause, which is not found in allowedClauses of def OMP_Atomic is caught in parsing stage.

A test for this might be good with this patch itself.

Thanks for the replies. No further questions from my side. I have already approved.

clementval added a comment.EditedOct 13 2020, 12:18 PM

Now coming to what standard 5.0 says for one of the constraints:

At most one memory-order-clause may appear on the construct.

So a test like

!$omp atomic SEQ_CST SEQ_CST read
     i  = j

Is currently not flagged as semantic error.
The possible reason which Valentine explained me was that "there should be at least a Enter function for each clauses with a CheckAllowed inside check-omp-structure.cpp".
And those functions lack in check-omp-structure.cpp which was my next patch.

This should be done with the allowedOnceClauses list in OMP.td nothing should be done in the check-omp-structure.cpp

This revision was landed with ongoing or failed builds.Oct 14 2020, 1:50 AM
This revision was automatically updated to reflect the committed changes.

A test for this might be good with this patch itself.

Thank you..
Test added with the commit into LLVM trunk.