[openmp] Use switch in isAllowedClauseForDirective instead of multiple if

[clementval]

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.

[jdoerfert]

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

471 ↗

(On Diff #276289)

[Drive By] Unrelated?

[clementval]

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

471 ↗

(On Diff #276289)

It was a duplicate. Already on line 469.

[clementval]

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

471 ↗

(On Diff #276289)

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

[jdoerfert]

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

471 ↗

(On Diff #276289)

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

[clementval]

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

471 ↗

(On Diff #276289)

Ok! Good to know!

[clementval]

Rebase

[jdenny]

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

253–254

Maybe GenerateCaseForVersionedClauses given that it's not just allowedClauses?

[clementval]

Address review comments

[clementval]

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

253–254

Yeah might be better and less confusing in the future.

[clementval]

Small fix to indent in lit test

[jdenny]

llvm/test/TableGen/directive1.td

124

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

[jdenny]

llvm/test/TableGen/directive1.td

124

This is what I'm thinking of:

http://llvm.org/docs/CodingStandards.html#don-t-use-default-labels-in-fully-covered-switches-over-enumerations

[clementval]

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]

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.

[clementval]

Remove correct default case and revert llvm_unreachable

[clementval]

In D83363#2139526, @jdenny wrote:

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.

[clementval]

Revert llvm_unreachable in test as well

[jdenny]

Still LGTM.

llvm/test/TableGen/directive1.td

 // IMPL-NEXT:        return "clauseb";
 // IMPL-NEXT:    }
 // IMPL-NEXT:    llvm_unreachable("Invalid Tdl Clause kind");
 // IMPL-NEXT:  }
 // IMPL-EMPTY:
 // IMPL-NEXT:  bool llvm::tdl::isAllowedClauseForDirective(Directive D, Clause C, unsigned Version) {
 // IMPL-NEXT:    assert(unsigned(D) <= llvm::tdl::Directive_enumSize);
 // IMPL-NEXT:    assert(unsigned(C) <= llvm::tdl::Clause_enumSize);
-// IMPL-NEXT:    if (D == TDLD_dira && C == TDLC_clausea && 1 <= Version && 2147483647 >= Version)
-// IMPL-NEXT:      return true;
-// IMPL-NEXT:    if (D == TDLD_dira && C == TDLC_clauseb && 1 <= Version && 2147483647 >= Version)
-// IMPL-NEXT:      return true;
-// IMPL-NEXT:    return false;
-// IMPL-NEXT:  }
+// IMPL-NEXT:    switch (D) {
+// IMPL-NEXT:      case TDLD_dira:
+// IMPL-NEXT:        switch (C) {
+// IMPL-NEXT:          case TDLC_clausea:
+// IMPL-NEXT:            return 1 <= Version && 2147483647 >= Version;
+// IMPL-NEXT:          case TDLC_clauseb:
+// IMPL-NEXT:            return 1 <= Version && 2147483647 >= Version;
+// IMPL-NEXT:          default:
+// IMPL-NEXT:            return false;
+// IMPL-NEXT:        }
+// IMPL-NEXT:        break;
+// IMPL-NEXT:    }
+// IMPL-NEXT:    llvm_unreachable("Invalid Tdl Directive kind");
+// IMPL-NEXT:  }

llvm/test/TableGen/directive2.td

 // IMPL-NEXT:        return "clauseb";
 // IMPL-NEXT:    }
 // IMPL-NEXT:    llvm_unreachable("Invalid Tdl Clause kind");
 // IMPL-NEXT:  }
 // IMPL-EMPTY:
 // IMPL-NEXT:  bool llvm::tdl::isAllowedClauseForDirective(Directive D, Clause C, unsigned Version) {
 // IMPL-NEXT:    assert(unsigned(D) <= llvm::tdl::Directive_enumSize);
 // IMPL-NEXT:    assert(unsigned(C) <= llvm::tdl::Clause_enumSize);
-// IMPL-NEXT:    if (D == TDLD_dira && C == TDLC_clausea && 2 <= Version && 4 >= Version)
-// IMPL-NEXT:      return true;
-// IMPL-NEXT:    if (D == TDLD_dira && C == TDLC_clauseb && 2 <= Version && 2147483647 >= Version)
-// IMPL-NEXT:      return true;
-// IMPL-NEXT:    return false;
-// IMPL-NEXT:  }
-
+// IMPL-NEXT:    switch (D) {
+// IMPL-NEXT:      case TDLD_dira:
+// IMPL-NEXT:        switch (C) {
+// IMPL-NEXT:          case TDLC_clausea:
+// IMPL-NEXT:            return 2 <= Version && 4 >= Version;
+// IMPL-NEXT:          case TDLC_clauseb:
+// IMPL-NEXT:            return 2 <= Version && 2147483647 >= Version;
+// IMPL-NEXT:          default:
+// IMPL-NEXT:            return false;
+// IMPL-NEXT:        }
+// IMPL-NEXT:        break;
+// IMPL-NEXT:    }
+// IMPL-NEXT:    llvm_unreachable("Invalid Tdl Directive kind");
+// IMPL-NEXT:  }

llvm/utils/TableGen/DirectiveEmitter.cpp

if (ImplicitAsUnknown && R->getValueAsBit("isImplicit")) {
       OS << "    .Case(\"" << Name << "\"," << Prefix << getFormattedName(Name)
          << ")\n";
     }
   }
   OS << "    .Default(" << Prefix << FormattedDefaultName << ");\n";
   OS << "}\n";
 }
 
-void GenerateTestForAllowedClauses(const std::vector<Record *> &Clauses,
+void GenerateCaseForVersionedClauses(const std::vector<Record *> &Clauses,
                                      raw_ostream &OS, StringRef DirectiveName,
                                      StringRef DirectivePrefix,
                                      StringRef ClausePrefix) {
-
-  const auto FormattedDirectiveName = getFormattedName(DirectiveName);
   for (const auto &C : Clauses) {
     const auto MinVersion = C->getValueAsInt("minVersion");
     const auto MaxVersion = C->getValueAsInt("maxVersion");
     const auto SpecificClause = C->getValueAsDef("clause");
     const auto ClauseName = SpecificClause->getValueAsString("name");
-
-    OS << "  if (D == " << DirectivePrefix << FormattedDirectiveName
-       << " && C == " << ClausePrefix << getFormattedName(ClauseName) << " && "
-       << MinVersion << " <= Version && " << MaxVersion << " >= Version)\n";
-    OS << "    return true;\n";
+    OS << "        case " << ClausePrefix << getFormattedName(ClauseName)
+       << ":\n";
+    OS << "          return " << MinVersion << " <= Version && " << MaxVersion
+       << " >= Version;\n";
   }
 }
 
 // Generate the isAllowedClauseForDirective function implementation.
 void GenerateIsAllowedClause(const std::vector<Record *> &Directives,
-                             raw_ostream &OS, StringRef DirectivePrefix,
-                             StringRef ClausePrefix, StringRef CppNamespace) {
+                             raw_ostream &OS, StringRef LanguageName,
+                             StringRef DirectivePrefix, StringRef ClausePrefix,
+                             StringRef CppNamespace) {
   OS << "\n";
   OS << "bool llvm::" << CppNamespace << "::isAllowedClauseForDirective("
      << "Directive D, Clause C, unsigned Version) {\n";
   OS << "  assert(unsigned(D) <= llvm::" << CppNamespace
      << "::Directive_enumSize);\n";
   OS << "  assert(unsigned(C) <= llvm::" << CppNamespace
      << "::Clause_enumSize);\n";
 
+  OS << "  switch (D) {\n";
+
   for (const auto &D : Directives) {
+
     const auto DirectiveName = D->getValueAsString("name");
 
+    OS << "    case " << DirectivePrefix << getFormattedName(DirectiveName)
+       << ":\n";
+    OS << "      switch (C) {\n";
+
     const auto &AllowedClauses = D->getValueAsListOfDefs("allowedClauses");
-    GenerateTestForAllowedClauses(AllowedClauses, OS, DirectiveName,
+    GenerateCaseForVersionedClauses(AllowedClauses, OS, DirectiveName,
                                     DirectivePrefix, ClausePrefix);
 
     const auto &AllowedOnceClauses =
         D->getValueAsListOfDefs("allowedOnceClauses");
-    GenerateTestForAllowedClauses(AllowedOnceClauses, OS, DirectiveName,
+    GenerateCaseForVersionedClauses(AllowedOnceClauses, OS, DirectiveName,
                                     DirectivePrefix, ClausePrefix);
 
     const auto &RequiredClauses = D->getValueAsListOfDefs("requiredClauses");
-    GenerateTestForAllowedClauses(RequiredClauses, OS, DirectiveName,
+    GenerateCaseForVersionedClauses(RequiredClauses, OS, DirectiveName,
                                     DirectivePrefix, ClausePrefix);
-  }
-  OS << "  return false;\n";
-  OS << "}\n";
+
+    OS << "        default:\n";
+    OS << "          return false;\n";
+    OS << "      }\n"; // End of clauses switch
+    OS << "      break;\n";
+  }
+
+  OS << "  }\n"; // End of directives switch
+  OS << "  llvm_unreachable(\"Invalid " << LanguageName
+     << " Directive kind\");\n";
+  OS << "}\n"; // End of function isAllowedClauseForDirective
 }
 
 // Generate the implemenation section for the enumeration in the directive
 // language
 void EmitDirectivesImpl(RecordKeeper &Records, raw_ostream &OS) {
 
   const auto &DirectiveLanguages =
       Records.getAllDerivedDefinitions("DirectiveLanguage");
void EmitDirectivesImpl(RecordKeeper &Records, raw_ostream &OS) {
   // getClauseKind(StringRef Str)
   GenerateGetKind(Clauses, OS, "Clause", ClausePrefix, LanguageName,
                   CppNamespace, /*ImplicitAsUnknown=*/true);
 
   // getClauseName(Clause Kind)
   GenerateGetName(Clauses, OS, "Clause", ClausePrefix, LanguageName,
                   CppNamespace);
 
-  GenerateIsAllowedClause(Directives, OS, DirectivePrefix, ClausePrefix,
-                          CppNamespace);
+  GenerateIsAllowedClause(Directives, OS, LanguageName, DirectivePrefix,
+                          ClausePrefix, CppNamespace);
 }
 
 } // namespace llvm