[PATCH] D128297: [pseudo] Track heads as GSS nodes, rather than as "pending actions".

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 6 inline comments as done.
sammccall added a comment.

As discussed offline, I'm going to land this patch as other approved optimizations are based on it, and there are no big objections.

Please feel free to keep discussing details here and I'll land followups.



================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:140
+              const ForestNode &NextTok, const ParseParams &Params,
+              std::vector<const GSS::Node *> &NewHeads);
+// Applies available reductions on Heads, appending resulting heads to the list.
----------------
hokein wrote:
> nit: change NewHeads to a pointer? it seems clearer that NewHeads is the output of the function from the caller side `glrShift(OldHeads, ..., &NewHeads)`.
> 
> I think it would be clearer if glrShift just returns NewHeads, but I understand we want to avoid temporary object for performance reasons. 
I think that's only clearer if there's a convention that references are immutable and not used for output, and I don't think that convention exists in LLVM.
Maybe there's some association, but using a pointer can also communicate other things (like nullability) that we don't intend here.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:69
+    std::swap(Heads, NextHeads);
+    SymbolID Lookahead = I + 1 == Terminals.size() ? tokenSymbol(tok::eof)
+                                                   : Terminals[I + 1].symbol();
----------------
hokein wrote:
> I'd probably leave a blank line deliberately after glrShift, because the glrReduce work on the *next* I+1 token.
I think we have different mental models here.

With loop index I:
 - glrShift consumes token I
 - glrReduce consumes no tokens, just rearranges things

I think of glrReduce as:
 a) not operating on a token, at least in the sense that glrShift does
 b) being part of the work for token I, not token I+1 (it's consuming stack items corresponding to token I)

It happens to **look ahead** at token I+1, but I see that as mostly incidental: with LR(0) we wouldn't do that, with SLR(2) we'd look at both token I+1 and I+2. Token I+1 isn't fundamentally important.

(I'm happy to add the blank line, but wanted to clarify because this is one reason I see this formulation as simpler)


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:111
+              const ForestNode &NewTok, const ParseParams &Params,
+              std::vector<const GSS::Node *> &NewHeads) {
   assert(NewTok.kind() == ForestNode::Terminal);
----------------
hokein wrote:
> nit: add a `NewHeads.empty` assertion? 
Why does this function care about that? It just appends to NewHeads.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:200
 //     0--5(pointer)       // 5 is goto(0, pointer)
-void glrReduce(std::vector<ParseStep> &PendingReduce, const ParseParams &Params,
-               NewHeadCallback NewHeadCB) {
+void glrReduce(std::vector<const GSS::Node *> &Heads, SymbolID Lookahead,
+               const ParseParams &Params) {
----------------
hokein wrote:
> IIRC, in glrReduce, we only append newly-created GSS nodes in Heads, and never to do deleting, so the Heads will end up with lots of unnecessary heads (heads created for reduces), and we will call `getShiftState` on them. 
> 
> We know that after glrReduce, active heads are heads with available shift actions, so we can optimize it further, we could just use a vector<GSS::Nodes> which just contains active heads , this could be done in the popPending. I think it will increase the performance by reducing the number of unnecessary heads.
Thanks for the offline discussion - I understand this better now!

You're right that at the moment we're doing a lookup that happens to yield this information. This is because shift + reduce info is stored in the same table.
This would make glrShift cheaper, which could be worth up to 9% now and up to 15% later (after glrReduce optimizations).

However D128318 splits these data structures apart to exploit different patterns in them (shift actions are sparser, reduces are denser but have patterns that allows them to be compressed).
I don't want to implement this now as that change is a bigger speedup (24%).

Fundamentally the idea is to avoid a single hashtable lookup. Storing one bit per (state, terminal) to see whether shift is possible is only 74kB, maybe a big std::bitset would work?

Added a FIXME to LRTable::getShiftState.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:264
+  // PoppedHeads is our position within it.
+  unsigned PoppedHeads = 0;
   // Pop walks up the parent chain(s) for a reduction from Head by to Rule.
----------------
hokein wrote:
> might be name it PoppedHeadIndex?
No, it points to the first head that *isn't* popped.
Renamed to NextPopHead


================
Comment at: clang-tools-extra/pseudo/lib/grammar/LRTable.cpp:78
+  assert(pseudo::isToken(Terminal) && "expected terminal symbol!");
+  for (const auto &Result : find(State, Terminal))
+    if (Result.kind() == Action::Shift)
----------------
hokein wrote:
> instead of using find directly, just use `getActions()`.
OK, but I think we should get rid of getActions soon.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128297