hokein added a comment.
Nice! I really like the form it goes now.
================
Comment at: clang-tools-extra/pseudo/include/clang-pseudo/GLR.h:82
// FIXME: Most nodes live a fairly short time, and are simply discarded.
// Is it worth refcounting them (we have empty padding) and returning to a
----------------
This FIXME is done by this patch :)
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:15
#include "llvm/ADT/STLExtras.h"
+#include "llvm/ADT/ScopeExit.h"
#include "llvm/ADT/StringExtras.h"
----------------
an off-topic comment: we only use the function in debug branch code, this will
trigger the include-cleaner warning in release mode :)
we could warp it with `#ifndef NDEBUG` as well.
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:87
NewHeads.clear();
+ MaybeGC();
glrReduce(PendingReduce, Params,
----------------
I would just call it before the `NewHeads.clear()`, and run the `gc` on the
`NewHeads` directly.
It simplifies the implementation (no need to traverse three pending actions,
and no duplicated elements in Roots). The downside is that some dead heads
(heads with no available actions on `Terminal.symbol()`) will not be GCed in
this run, but I think it is negligible, as those will be GCed in next run.
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:391
+ llvm::ArrayRef<const Node *> Parents) {
+ ++NodeCount;
+ Node *Result = new (allocate(Parents.size()))
----------------
The NodeCount becomes vague now, we update in both addNode and allocate
methods, I think we should have two separate counts.
================
Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:403
+ ++NodeCount;
+ if (FreeList.size() <= Parents)
+ FreeList.resize(Parents + 1);
----------------
What's the practical size of the `FreeList`? I think we can just find one, and
use it as initial default size, to save some cost of resize.
================
Comment at: clang-tools-extra/pseudo/unittests/GLRTest.cpp:412
+ EXPECT_EQ(0u, GSStack.gc({AB, C})); // D is already gone.
+ auto *E = GSStack.addNode(0, nullptr, {Root});
+ EXPECT_EQ(3u, GSStack.gc({A, E})); // destroys B, AB, C
----------------
nit: since we free D, I think here E will reuse the memory free from D, maybe
add pointer comparison with `E` and `D` ?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D126723/new/
https://reviews.llvm.org/D126723
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
