Page MenuHomePhabricator

[Flang][OpenMP][NFC][1/2] Reorder OmpStructureChecker and simplify it.
ClosedPublic

Authored by sameeranjoshi on Wed, Oct 28, 11:20 AM.

Details

Summary

OmpStructureChecker has too much boilerplate code in source file.
It was not easy to figure out the seperation of clauses inside 'OmpClause' and
the ones which had a seperate node in parse-tree.h.

This patch:

  1. Removes the boilerplate by defining a few macros.
  2. Makes seperation between constructs, directives and clauses(sub classes are seperated).
  3. Macros could have been shared between OMP and OACC, template specilizations might have been costly hence used macros.

Follows the same strategy used for AccStructureChecker.

Next patch in series to simplify OmpStructureChecker would try to simplify
boilerplates inside the functions and either create abstractions or use if
something is available inside check-directive-structure.h

Diff Detail

Event Timeline

sameeranjoshi created this revision.Wed, Oct 28, 11:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Oct 28, 11:20 AM
sameeranjoshi requested review of this revision.Wed, Oct 28, 11:20 AM
sameeranjoshi added a project: Restricted Project.
clementval added a comment.EditedWed, Oct 28, 11:44 AM

We might even go further and generate these with TableGen. I guess we have most of the information in OMP.td or ACC.td for that. Anyway might be wise to first land this patch and look at TableGen in a next step. Would be happy to look at it if it makes sense.

Anyway LGTM. Will let OpenMP folks comment and accept it.

We might even go further and generate these with TableGen. I guess we have most of the information in OMP.td or ACC.td for that.
Anyway might be wise to first land this patch and look at TableGen in a next step. Would be happy to look at it if it makes sense.

+1

Makes seperation between constructs, directives and clauses(sub classes are seperated).

Is this change in this patch?

flang/lib/Semantics/check-omp-structure.cpp
21

The name is a bit misleading since it does CheckAllowed also.

373–407

Nit: seperate -> separate. Used in a few other places and in the summary.

Makes seperation between constructs, directives and clauses(sub classes are seperated).

Is this change in this patch?

Yes, the file has

  1. Constructs and directives at the beginning.
  2. Clauses in the later half. The clauses are themselves divided as
    1. First half has clauses which are inside OmpClause.
    2. Second half for clauses which have a special node in parse-tree.h. e.g OmpMemoryOrderClause and similar ones might come here.

Before this patch they were somewhat mixed in the file.

flang/lib/Semantics/check-omp-structure.cpp
21

I see CheckAllowed used at all places, hence that is assumed as that is necessary.
CHECK_REQ_SCALAR_INT_CLAUSE is as well shared with AccStructureChecker.
Do you have a better suggestion here?

Thank you for suggestion.

May be it seems possible.
What benefit do you see compared to the current approach?
I see we can group all clauses at one location, compared to the one I mentioned below.

Anticipating the status of OMP/OACC for the next 4 months I think the macros would be replaced with Enter/Leave functions with more checks in them from standard.
for e.g something like Enter(const parser::OmpClause::Ordered &x)
This function has other restrictions implemented from standard specific to the clause apart from tablegen changes.

So the macros are somewhat partial checks, having macros makes us easily figure out in this case whether the restrictions on clause are partially implemented or more detailed.

What benefit do you see compared to the current approach?

We cannot put a breakpoint in macros.

So the macros are somewhat partial checks, having macros makes us easily figure out in this case whether the restrictions on clause are partially implemented or more detailed.

I don't fully agree on this one. At least on the OpenACC side lots of clauses will have only those checks and thus keep the macro or TableGen style.

So the macros are somewhat partial checks, having macros makes us easily figure out in this case whether the restrictions on clause are partially implemented or more detailed.

I don't fully agree on this one. At least on the OpenACC side lots of clauses will have only those checks and thus keep the macro or TableGen style.

IIUC, TableGen would check only for allowed, notallowed, etc and the restrictions in TableGen.

Where would the other restrictions be checked?
e.g Enter(const parser::OmpScheduleClause &x) there are too many checks.

Are they done somewhere in OACC which I might be unaware of ?

IIUC, TableGen would check only for allowed, notallowed, etc and the restrictions in TableGen.

Where would the other restrictions be checked?
e.g Enter(const parser::OmpScheduleClause &x) there are too many checks.

That's not what I meant. What I meant is that lots of clauses have just trivial checks and therefore will just use the macros and nothing else. In your statement your were mentioning that those were just temporary but in my opinion (at least on the OpenACC side) they will stay as is since they do not have further restrictions.

IIUC, TableGen would check only for allowed, notallowed, etc and the restrictions in TableGen.

Where would the other restrictions be checked?
e.g Enter(const parser::OmpScheduleClause &x) there are too many checks.

That's not what I meant. What I meant is that lots of clauses have just trivial checks and therefore will just use the macros and nothing else. In your statement your were mentioning that those were just temporary but in my opinion (at least on the OpenACC side) they will stay as is since they do not have further restrictions.

True for OpenACC, but OMP has restrictions on clauses apart from trivial checks(I haven't checked the standard on which clauses might be exclusion to this statement), but at least what I can see from the patches which @yhegde and @praveen pushed they mostly implement these clauses.

What should be the final decision on this patch finally?

clementval accepted this revision.Mon, Nov 2, 11:13 AM

What should be the final decision on this patch finally?

Looks fine for me but wait for someone closer to OpenMP to double approve it.

This revision is now accepted and ready to land.Mon, Nov 2, 11:13 AM
kiranchandramohan accepted this revision.Mon, Nov 2, 3:02 PM

Thanks for the cleanup. If you can remove the macros in a later patch that would be great.