sammccall added inline comments.
================ 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" ---------------- hokein wrote: > 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. conditional including is ugly and can cause subtle opt-only compile failures. I'm tempted to add a "keep" pragma but want to think about how to handle this case a bit more. ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:87 NewHeads.clear(); + MaybeGC(); glrReduce(PendingReduce, Params, ---------------- hokein wrote: > 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. Oops, I forgot that Pending* are all empty at the beginning of this loop. I should probably add an assertion for that. We can call it right at the top of the loop, yes? It doesn't need to be between AddSsteps and NewHeads.clear(). > run the gc on the NewHeads directly Unfortunately not: the GC naturally consumes the argument (it becomes the stack for the DFS) so we need a copy. We could move the copy into gc() but: - this way we can reuse the same vector every time and don't have to allocate - soon we'll have error-recovery candidates as additional roots we want to pass in ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:391 + llvm::ArrayRef<const Node *> Parents) { + ++NodeCount; + Node *Result = new (allocate(Parents.size())) ---------------- hokein wrote: > The NodeCount becomes vague now, we update in both addNode and allocate > methods, I think we should have two separate counts. - Oops, that duplicated ++NodeCount is just a mistake. - names are vague now, both "NodeCount" and "Allocated" are dubious. Maybe I'll rename to `NodesCreated` and `Alive`, WDYT? - are there actually more metrics we want to record? we have nodes-created, currently-live-nodes (Allocated.size()), nodes-destroyed (`NodeCount - Allocated.size()`). We could track max-live-node but I think Arena.bytes() is fine for our purposes. - do we want to expose any of those numbers through APIs? I couldn't work out what I'd actually do with them. ================ Comment at: clang-tools-extra/pseudo/lib/GLR.cpp:403 + ++NodeCount; + if (FreeList.size() <= Parents) + FreeList.resize(Parents + 1); ---------------- hokein wrote: > 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. It's whatever the max #parents is - maybe 10 or something? I don't think it's worth adding a hard-coded guess to save ~1 resize of a single vector per parse! 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