[PATCH] D128299: [pseudo] Add a fast-path to GLR reduce when both pop and push are trivial

[Sam McCall via Phabricator via cfe-commits]

sammccall marked 2 inline comments as done.
sammccall added inline comments.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:269
+  // bypass all these fancy queues and pop+push directly. This is very common.
+  auto PopAndPushTrivial = [&]() -> bool {
+    if (!Sequences.empty() || Heads.size() != PoppedHeads + 1)
----------------
hokein wrote:
> IMO, it is exactly what the reduction implementation would look like for a linear LR parsing :)
> 
> maybe encode the linear in the name?
Thanks for making this connection, I hadn't completely realized we're just acting like an LR parser here and fundamentally that's why it's cheap.

I've amended the comment to call this out explicitly.

I'm not sure changing the *name* is better though, because the name has some jobs to do at the callsite:
 - convey that we're non just popping but pushing also (which is suprising)
 - suggest that this is handling some simple cases, but not the general case

I don't think "linear" actually conveys the second point. It partially describes *which* simple cases are handled.
So it would need to be tacked on like `PopAndPushTrivialLinear` and to me that's enough concepts that my brain has trouble digesting it.

This would be worth it if it were a critical part of the interface, but I don't think it is - it's rather important to the implementation instead.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:270
+  auto PopAndPushTrivial = [&]() -> bool {
+    if (!Sequences.empty() || Heads.size() != PoppedHeads + 1)
+      return false;
----------------
hokein wrote:
> The patch description is really nice document, and help to me to justify the code. I'd suggest adding them in the comment as well.
> 
> > there must be no pending pushes and only one head available to pop
> > the head must have only one reduction rule
> > the reduction path must be a straight line (no multiple parents)
> 
> 
Done. The new comment is based on the patch descriptions, with amendments to talk about the relationship to LR you pointed out.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:320
     for (; PoppedHeads < Heads.size(); ++PoppedHeads) {
-      // FIXME: if there's exactly one head in the queue, and the pop stage
-      // is trivial, we could pop + push without touching the expensive queues.
+      if (PopAndPushTrivial())
+        continue;
----------------
hokein wrote:
> This seems very clever -- for trivial case, the main reduce loop is happening here.
Can't tell if "clever" is a good or bad thing :-)

Added a comment, it's definitely not obvious.


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:323
       for (const auto &A :
            Params.Table.getActions(Heads[PoppedHeads]->State, Lookahead)) {
         if (A.kind() != LRTable::Action::Reduce)
----------------
hokein wrote:
> we could save an extra call of `Params.Table.getActions` -- we call it at the beginning of the loop body and store the results in a local var, and use it in PopAndPushTrivial and here.
Yes, I benchmarked this and was surprised to see no difference at all!

I'd be tempted to do it anyway, but D128318 obsoletes this idea entirely, by making reduce lookup extremely cheap. At that point it's not worth messing up the signatures.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128299