This is an archive of the discontinued LLVM Phabricator instance.

[openmp] Use switch in isAllowedClauseForDirective instead of multiple if
ClosedPublic

Authored by clementval on Jul 7 2020, 5:55 PM.

Details

Summary

Change the test in isAllowedClauseForDirective from if with multiple conditions
to a main switch on directive and then switches on clause for each directive. Version
check is still done with a condition in the return statment.

Diff Detail

Event Timeline

clementval created this revision.Jul 7 2020, 5:55 PM
jdoerfert added inline comments.Jul 7 2020, 7:12 PM
llvm/include/llvm/Frontend/OpenMP/OMP.td
471

[Drive By] Unrelated?

clementval marked 2 inline comments as done.Jul 7 2020, 7:14 PM
clementval added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMP.td
471

It was a duplicate. Already on line 469.

clementval marked 2 inline comments as done.Jul 7 2020, 7:14 PM
clementval added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMP.td
471

Makes compilation error because of duplicate case when code is generated.

jdoerfert added inline comments.Jul 7 2020, 7:15 PM
llvm/include/llvm/Frontend/OpenMP/OMP.td
471

Commit NFC changes like that w/o review if it "just makes sense".

clementval marked 2 inline comments as done.Jul 7 2020, 7:17 PM
clementval added inline comments.
llvm/include/llvm/Frontend/OpenMP/OMP.td
471

Ok! Good to know!

clementval updated this revision to Diff 276302.Jul 7 2020, 7:40 PM
clementval marked an inline comment as done.

Rebase

jdenny added inline comments.Jul 8 2020, 7:08 AM
llvm/test/TableGen/directive1.td
120

In most examples I've seen, the outer case would be formatted as:

case TDLD_dira: {
  . . .
  break;
}

clang-format puts the { on the same line as the case.

grep shows some code putting break; on the same line as the } and some code putting it on the line before. However, I did at least find one example in LLVM docs showing the way I'm suggesting above.

Alternatively, as in this example, couldn't those braces be dropped given that there are no local declarations?

124

The unreachable message doesn't make sense given the default in the directive switch. If that switch covers all directives, default isn't needed anyway.

llvm/utils/TableGen/DirectiveEmitter.cpp
255

Maybe GenerateCaseForVersionedClauses given that it's not just allowedClauses?

clementval updated this revision to Diff 276441.Jul 8 2020, 8:09 AM
clementval marked 6 inline comments as done.

Address review comments

Thanks for the review. I just updated the patch.

llvm/test/TableGen/directive1.td
120

I guess we can drop the braces and then it's clear where the break; is going.

124

Will remove it.

llvm/utils/TableGen/DirectiveEmitter.cpp
255

Yeah might be better and less confusing in the future.

clementval updated this revision to Diff 276442.Jul 8 2020, 8:16 AM

Small fix to indent in lit test

jdenny added inline comments.Jul 8 2020, 8:28 AM
llvm/test/TableGen/directive1.td
124

Is the default useful? Are all directives covered by cases?

clementval updated this revision to Diff 276472.Jul 8 2020, 9:58 AM
clementval marked 3 inline comments as done.

Remove default in directive switch

llvm/test/TableGen/directive1.td
124

Yeah for directive we get remove it since it's fully covered. Just pushed an update.

jdenny accepted this revision.Jul 8 2020, 10:18 AM

Other than the last bit of cleanup I commented on, LGTM.

llvm/test/TableGen/directive1.td
124

But it needs llvm_unreachable, whose message makes sense now that default is removed.

Also, the last update removed the wrong default from the emitter.

This revision is now accepted and ready to land.Jul 8 2020, 10:18 AM
clementval marked 2 inline comments as done.

Remove correct default case and revert llvm_unreachable

Other than the last bit of cleanup I commented on, LGTM.

Should be good now.

llvm/test/TableGen/directive1.td
124

Of course! Shouldn't try to do two things at the same time ... sorry for the mix.

Revert llvm_unreachable in test as well

jdenny accepted this revision.Jul 8 2020, 12:19 PM

Still LGTM.

Harbormaster completed remote builds in B63453: Diff 276497.
This revision was automatically updated to reflect the committed changes.