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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
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.
----------------
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. 


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:220
     const GSS::Node* Base = nullptr;
-    Sequence Seq;
+    Sequence *Seq;
   };
----------------
nit: add a default value nullptr


================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:270
+
+        Sequences.emplace(F, PushSpec{N, Seq});
         return;
----------------
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()`).


================
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(
----------------
What's XXX?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128307