This is an archive of the discontinued LLVM Phabricator instance.

[openmp] Move isAllowedClauseForDirective to tablegen + add clause version to OMP.td
ClosedPublic

Authored by clementval on Jul 1 2020, 9:41 AM.

Details

Summary

Generate the isAllowedClauseForDirective function from tablegen. This patch introduce
the VersionedClause in the tablegen file so that clause can be encapsulated in this class to
specify a range of validity on a directive.

VersionedClause has default minVersion, maxVersion so it can be used without them or
minVersion.

Diff Detail

Event Timeline

clementval created this revision.Jul 1 2020, 9:41 AM

MIssing new line at the end of llvm/test/TableGen/directive2.td

clementval edited the summary of this revision. (Show Details)Jul 1 2020, 12:27 PM

I browsed over it and it looks great. Thanks.

@jdenny can you take a look and do the review :)

Thanks for working on this.

llvm/include/llvm/Frontend/Directive/DirectiveBase.td
64

Please add punctuation to all comments.

65

Why not unsigned as in the original?

69

What does "the list" refer to?

llvm/include/llvm/Frontend/OpenMP/OMP.td
295

The closing ] is inconsistently placed here and in some other cases.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
1881

This patch loses this TODO and the next one. I'm not sure what they mean. Do we need to keep them?

llvm/test/TableGen/directive1.td
109

I know the original code used a giant if-else block, but shouldn't this be a switch?

llvm/utils/TableGen/DirectiveEmitter.cpp
55–56

Any reason not to call getFormattedName here instead of twice below?

182–184

Call getFormattedName(DefaultName) once here?

215

Hoist getFormattedName(DirectiveName) out of the loop?

224

I'd drop this comment's last two sentences, which don't seem like they will be meaningful after pushing.

297

Why is an empty line needed?

clementval marked 19 inline comments as done.

Address review comments

Thanks for the review @jdenny. The patch is updated with your comments and some comments as well.

llvm/include/llvm/Frontend/Directive/DirectiveBase.td
65

TableGen type does not have an unsigned type so I went with the closest one.

69

I updated the comment and dropped the list. I think it is clearer now.

llvm/include/llvm/Frontend/OpenMP/OMP.td
295

Good catch. I updated the file as well.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
1881

I updated the patch to keep it. Same for depobj.

llvm/test/TableGen/directive1.td
109

The idea was to migrate to TableGen with the exact same code generated and to update this in a follow up patch. It would be replace with at least a switch for the Directive and probably an inner switch for the Clause or we keep the ifs inside the Directives' switch.

llvm/utils/TableGen/DirectiveEmitter.cpp
55–56

Updated in the patch.

182–184

Updated as well.

297

Just to be consistent with clang-format in the generated file.

jdenny marked 2 inline comments as done.Jul 6 2020, 2:14 PM
jdenny added inline comments.
llvm/include/llvm/Frontend/Directive/DirectiveBase.td
65

Ah, I thought I had seen it used elsewhere, but it looks like I'm mistaken.

69

I don't see the change.

llvm/include/llvm/Frontend/OpenMP/OMPKinds.def
1881

@jdoerfert : If I read git blame correctly, you wrote this TODO originally. Can you help us to understand whether it's worth preserving?

llvm/test/TableGen/directive1.td
109

Sure, makes sense to do it in a separate patch.

It seems like both switches makes sense, but the inner switches might benefit from default cases.

llvm/utils/TableGen/DirectiveEmitter.cpp
297

It's surprising that clang-format would require an empty line at the end of the file. Any idea why?

clementval updated this revision to Diff 275837.Jul 6 2020, 2:45 PM
clementval marked 4 inline comments as done.

Address more comment from @jdenny

llvm/include/llvm/Frontend/Directive/DirectiveBase.td
69

Forgot this one. Sorry.

llvm/utils/TableGen/DirectiveEmitter.cpp
297

Sorry, it is not clang-format but Phabricator which signal a missing empty line at end of file. This is not mandatory and can be removed. The file is generated. I just wanted to be consistent with the style.

jdenny added inline comments.Jul 6 2020, 3:15 PM
llvm/utils/TableGen/DirectiveEmitter.cpp
297

Are you referring to, for example, the version of directive2.td on the left side of this diff? Here, phabricator points out that the final line in the file is not newline-terminated. I don't recall seeing phabricator suggest an additional empty line.

Likewise, in EmitDirectivesImpl, the final GenerateIsAllowedClause call already newline-terminates the final line of the generated file, so I don't see a need for an additional empty line.

clementval marked 2 inline comments as done.Jul 6 2020, 3:20 PM
clementval added inline comments.
llvm/utils/TableGen/DirectiveEmitter.cpp
297

Your are correct. I removed this since it was a confusion on my side.

clementval updated this revision to Diff 275850.Jul 6 2020, 3:21 PM
clementval marked an inline comment as done.

Rebase + remove useless new line generation

jdenny accepted this revision.Jul 6 2020, 3:31 PM

LGTM. Thanks for addressing my concerns.

The TODOs we talked about can be handled later, once @jdoerfert replies.

This revision is now accepted and ready to land.Jul 6 2020, 3:31 PM
jdenny added a comment.Jul 6 2020, 3:45 PM

The TODOs we talked about can be handled later, once @jdoerfert replies.

To be clear, by "later", I meant another patch. I think this is ready to land.

This revision was automatically updated to reflect the committed changes.

The TODOs we talked about can be handled later, once @jdoerfert replies.

FWIW, you seem to have a pretty good handle on this (and I am a bit behind in my review list), thanks!