[webkit-reviews] review granted: [Bug 170111] B3::fixSSA should do liveness pruning : [Attachment 305443] the patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Filip Pizlo <fpizlo at apple.com>'s
request for review:
Bug 170111: B3::fixSSA should do liveness pruning
https://bugs.webkit.org/show_bug.cgi?id=170111

Attachment 305443: the patch

https://bugs.webkit.org/attachment.cgi?id=305443&action=review




--- Comment #6 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 305443
  --> https://bugs.webkit.org/attachment.cgi?id=305443
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305443&action=review

r=me. I mostly have some style comments/suggestions.

> Source/JavaScriptCore/ChangeLog:13
> +	   This makes B3::fixSSA run twice as fast. This is a 13% progression
on WasmBench compile
> +	   times.

Awesome!

> Source/JavaScriptCore/b3/air/AirCFG.h:44
> +    typedef BasicBlock* Node;
> +    typedef IndexSet<BasicBlock> Set;
> +    template<typename T> using Map = IndexMap<BasicBlock, T>;
> +    typedef Vector<BasicBlock*, 4> List;

Style: The typedef's here can all be "using"s. I feel like I've been seeing
this more in JSC/WebKit, and have been using it more myself.
(Same for typedefs elsewhere in this patch.)

> Source/JavaScriptCore/b3/air/AirCFG.h:61
> +    Node node(unsigned index) const { return m_code[index]; }
> +    unsigned numNodes() const { return m_code.size(); }

Style: Maybe call this block() and numBlocks()? (And make the interface changes
in WTF).

> Source/WTF/wtf/Liveness.h:36
> +// HEADS UP: The algorithm here is duplicated in AirRegLiveness.h. That one
uses sets rather
> +// than fancy vectors, because that's better for register liveness analysis.

Style: It feels weird to have a comment like this in WTF. Maybe we should have
the "heads up" inside AirRegLiveness?

> Source/WTF/wtf/Liveness.h:54
> +	       typename CFG::Node block = m_cfg.node(blockIndex);

Style: You use `typename CFG::Node` many times in this file, perhaps it's worth
adding something like `using Node = typename CFG::Node` at the top of this
class?

> Source/WTF/wtf/Liveness.h:246
> +		   m_block, instIndex - 1,

Don't you need to make sure instIndex is > 0? (Perhaps the code gracefully
handles the wrap around, but it feels more sane to make sure it's not zero.)