[PATCH] D128307: [pseudo] Store reduction sequences by pointer in heaps, instead of by value.

[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:208
+  // Underlying storage for sequences pointed to by stored SequenceRefs.
+  std::deque<Sequence> SequenceStorage;
+  // We don't actually destroy the sequences between calls, to reuse storage.
----------------
hokein wrote:
> I start to have a nervous feeling that we're going to add more and more intermediate data structure, which increases the complexity of the code, but I think the improvement is large enough to justify. 
Agree :-(


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:270
+
+        Sequences.emplace(F, PushSpec{N, Seq});
         return;
----------------
hokein wrote:
> Just an idea (no action required)
> 
> If we want to do further optmization, here if a sequence have multiple Bases, we will have multiple elements in `Sequences` -- they have the form of (F, PushSpec{N, Seq}) where only N (the base node) is different. 
> 
> - if we change the PushSpec::Base to store the child node of Base, we could save a bunch of space in Sequences. (The base can be retrieved dynamically from the `child->parents()`).
This is interesting! Essentially the reason we do the DFS eagerly (and pay to store its results) is we have to establish the Family via the start token, but this means we have to go only as far as the end of the sequence, rather than one further. (One further feels "cleaner" somehow, but who cares...)

I'll give this a try (not in this patch)


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:317
       FamilySequences.emplace_back(Sequences.top().first.Rule,
-                                   Sequences.top().second.Seq);
+                                   *Sequences.top().second.Seq); // XXX
       FamilyBases.emplace_back(
----------------
hokein wrote:
> What's XXX?
Oops, this was left-over from when Sequences stored by pointer and FamilySequences stored by value. Removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128307