[PATCH] D122303: [pseudo] Sort nonterminals based on their reduction order.

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Nice!



================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:55
     };
     for (const auto &Spec : Specs) {
       Consider(Spec.Target);
----------------
hokein wrote:
> sammccall wrote:
> > You could identify dependencies here, and then use them to sort the uniquenonterminals before  allocating SymbolIDs
> not sure I get the your point -- we could move the topological sort here, but it doesn't seem to be significantly better compared with the current solution.
(This related to the suggestion of sorting *symbols*, in which case you want to do it based on the specs, before assigning symbol IDs. It's obsolete now)


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:91
            "Too many rules to fit in RuleID bits!");
-    llvm::sort(T->Rules, [](const Rule &Left, const Rule &Right) {
-      // Sorted by the Target.
-      return std::tie(Left.Target, Left.Size) <
-             std::tie(Right.Target, Right.Size);
-    });
-    RuleID RulePos = 0;
+    const auto &TopologicalOrder = getTopologicalOrder(T.get());
+    llvm::stable_sort(
----------------
I'd call the variable SymbolOrder - once we've computed the order and decided to use it for symbols, it's no longer locally relevant that it's topological


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:104
+      while (End < T->Rules.size() &&
+             TopologicalOrder[T->Rules[End].Target] == TopologicalOrder[SID])
+        ++End;
----------------
You don't need TopologicalOrder[] wrapping both, just compare symbol IDs?


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:119
+  //
+  // It returns a "lookup" vector where the index is SymbolID, and the value is
+  // the index of the SymbolID in the topological-orderred array.
----------------
This explanation is a bit confusing, because it refers to a second array doesn't exist and never will.

Maybe "This function returns the sort key for each symbol, the array is indexed by SymbolID."

(I think we're actually returning the inverse of the chosen permutation, but that's not a helpful explanation :-)


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:142
+        Diagnostics.push_back(
+            llvm::formatv("The grammar is cyclic, see symbol {0}",
+                          T->Nonterminals[SID].Name));
----------------
The grammar contains a cycle involving symbol {0}?


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:147
+      VisitStates[SID] = Visiting;
+      auto It = llvm::partition_point(
+          Dependencies, [&SID](const std::pair<SymbolID, SymbolID> &D) {
----------------
this is more tersely `llvm::lower_bound(Dependencies, {SID, 0})`, making use of the fact that `std::less` will do what we want

And with the inline lambda is gone, it seems more idiomatic as a for loop:
```
for (auto It = llvm::lower_bound(...); It != Dependencies.end() && It->first == SID; ++It)
  DFS(It->second);
```


================
Comment at: clang-tools-extra/pseudo/lib/GrammarBNF.cpp:156
+      VisitStates[SID] = Visited;
+      Order.push_back(SID);
+    };
----------------
Instead of building up the permutation array and then inverting it afterwards, maybe directly `Result[SID] = NextKey++;` here?

(Having pre-sized the vector)


================
Comment at: clang-tools-extra/pseudo/unittests/GrammarTest.cpp:51
+      return G->table().Nonterminals[id(NonterminalName)].RuleRange.Start;
+    ADD_FAILURE() << "Expected a single rule for " << NonterminalName
+                  << ", but it has " << RuleRange.End - RuleRange.Start
----------------
nit: maybe "ruleFor() expected a single rule..." to make it clear this isn't a real grammar requirement


================
Comment at: clang-tools-extra/pseudo/unittests/GrammarTest.cpp:53
+                  << ", but it has " << RuleRange.End - RuleRange.Start
+                  << " rule!\n";
+    return 0;
----------------
nit: rule -> rules


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D122303/new/

https://reviews.llvm.org/D122303