Bug 125530 – DFG should have a separate StoreBarrier node

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

This is in preparation for GenGC. We use a separate node instead of making them implicitly part of other nodes so that it's easier to run analyses on them, e.g. for barrier elision. They will be inserted during the fixup phase. Initially they will not generate any code.

[Mark Hahnenberg]

Created attachment 219431 [details]
Patch

[Mark Hahnenberg]

Comment on attachment 219431 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5514
> +#if USE(JSVALUE64)
> +    jit.rshift64(MacroAssembler::TrustedImm32(3 + 4), scratch2);
> +#else
> +    jit.rshift32(MacroAssembler::TrustedImm32(3 + 4), scratch2);
> +#endif

Get rid of magic numbers. I'll probably add constants to MarkedBlock for both the 3 (markByteShiftAmount) and the 4 (atomShiftAmount).

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5523
> +    size_t markIndex = (reinterpret_cast<size_t>(owner) & ~MarkedBlock::blockMask) >> (3 + 4);

Ditto.

[EFL EWS Bot]

Comment on attachment 219431 [details]
Patch

Attachment 219431 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/49928267

[Mark Hahnenberg]

Created attachment 219433 [details]
Patch

[Oliver Hunt]

Comment on attachment 219431 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:407
> +                    node->convertToPhantom();

The release assert means you should never hit this

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:554
> -                fixEdge<KnownCellUse>(child1);
>                  fixEdge<Int32Use>(child2);
> +                fixEdge<KnownCellUse>(child1);

Why this reordering?

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5659
> +#if CPU(X86)
> +    jit.push(scratch1);
> +    jit.push(scratch1);
> +#endif
> +

This unaligns the stack in the subsequent call doesn't it?  the return address will be pushed to the stack leading to 3*sizeof(void*) alignment (i can't remember the x86 calling convention at all)

[Mark Hahnenberg]

(In reply to comment #5)
> (From update of attachment 219431 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219431&action=review
> 
> > Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp:407
> > +                    node->convertToPhantom();
> 
> The release assert means you should never hit this
> 
> > Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:554
> > -                fixEdge<KnownCellUse>(child1);
> >                  fixEdge<Int32Use>(child2);
> > +                fixEdge<KnownCellUse>(child1);
> 
> Why this reordering?
No reason, just cruft I forgot to undo. Will revert.

> 
> > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:5659
> > +#if CPU(X86)
> > +    jit.push(scratch1);
> > +    jit.push(scratch1);
> > +#endif
> > +
> 
> This unaligns the stack in the subsequent call doesn't it?  the return address will be pushed to the stack leading to 3*sizeof(void*) alignment (i can't remember the x86 calling convention at all)
I think that's correct. I was doing stack adjustments to make sure things were aligned, but I removed it and forgot to change this.

[EFL EWS Bot]

Comment on attachment 219433 [details]
Patch

Attachment 219433 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/50118231

[Filip Pizlo]

Comment on attachment 219433 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1592
> +        Node* storeNode = node->storeNode();

I really *really* don't like this. At all. The store barrier should not point to the "store" node. Instead, you should only plant a store barrier in fix up if you know that the store node needs one. Thereafter, the store barrier node should always unconditionally mean that you have a store barrier. 

The way you've done it currently is super fragile and I don't think we want it.

[Mark Hahnenberg]

(In reply to comment #8)
> (From update of attachment 219433 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=219433&action=review
> 
> > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1592
> > +        Node* storeNode = node->storeNode();
> 
> I really *really* don't like this. At all. The store barrier should not point to the "store" node. Instead, you should only plant a store barrier in fix up if you know that the store node needs one. Thereafter, the store barrier node should always unconditionally mean that you have a store barrier. 
> 
> The way you've done it currently is super fragile and I don't think we want it.

Why exactly is it so fragile?

[Filip Pizlo]

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 219433 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=219433&action=review
> > 
> > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1592
> > > +        Node* storeNode = node->storeNode();
> > 
> > I really *really* don't like this. At all. The store barrier should not point to the "store" node. Instead, you should only plant a store barrier in fix up if you know that the store node needs one. Thereafter, the store barrier node should always unconditionally mean that you have a store barrier. 
> > 
> > The way you've done it currently is super fragile and I don't think we want it.
> 
> Why exactly is it so fragile?

There are multiple places that not only have to know what a StoreBarrier is, but also, how StoreBarrier ought to behave depending on its storeNode().  That's fragile: imagine changing the meaning of one of the store nodes, or adding a new store node; you'll have to add it in all of the places that StoreBarrier gets handled.  Also imagine adding a new phase that tries to understand what various nodes do; now you'll have to make sure you take care of StoreBarrier.  You've already done this wrong in this patch: Clobberize appears to assume that StoreBarrier always reads/writes BarrierState even though StoreBarrier does *nothing* for some kinds of storeNode().

The worst part about this complexity that you've added is that it accomplishes nothing.  Why can't you just *not emit a StoreBarrier* in fixup phase if the store node doesn't actually do a barrier-able store?  Why do you have to have each phase that inspects a StoreBarrier have to again recompute whether or not the fixup phase should have inserted the StoreBarrier to begin with?

The whole point of the fixup phase is to minimize this kind of madness.  Just change fixup phase to not insert a StoreBarrier if it isn't needed, and then you'll be able to delete a bunch of the code in this patch.

[Filip Pizlo]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > (From update of attachment 219433 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=219433&action=review
> > > 
> > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1592
> > > > +        Node* storeNode = node->storeNode();
> > > 
> > > I really *really* don't like this. At all. The store barrier should not point to the "store" node. Instead, you should only plant a store barrier in fix up if you know that the store node needs one. Thereafter, the store barrier node should always unconditionally mean that you have a store barrier. 
> > > 
> > > The way you've done it currently is super fragile and I don't think we want it.
> > 
> > Why exactly is it so fragile?
> 
> There are multiple places that not only have to know what a StoreBarrier is, but also, how StoreBarrier ought to behave depending on its storeNode().  That's fragile: imagine changing the meaning of one of the store nodes, or adding a new store node; you'll have to add it in all of the places that StoreBarrier gets handled.  Also imagine adding a new phase that tries to understand what various nodes do; now you'll have to make sure you take care of StoreBarrier.  You've already done this wrong in this patch: Clobberize appears to assume that StoreBarrier always reads/writes BarrierState even though StoreBarrier does *nothing* for some kinds of storeNode().
> 
> The worst part about this complexity that you've added is that it accomplishes nothing.  Why can't you just *not emit a StoreBarrier* in fixup phase if the store node doesn't actually do a barrier-able store?  Why do you have to have each phase that inspects a StoreBarrier have to again recompute whether or not the fixup phase should have inserted the StoreBarrier to begin with?
> 
> The whole point of the fixup phase is to minimize this kind of madness.  Just change fixup phase to not insert a StoreBarrier if it isn't needed, and then you'll be able to delete a bunch of the code in this patch.

But, broadly, I think you're taking the wrong approach to DFG IR.  Store barriers are a *simple* concept.  They shouldn't require adding a whole new kind of node->node edge to the DFG IR.  A new opcode (StoreBarrier) is understandable.  But a new way in which nodes refer to each other is definitely overkill for such a simple thing.

Do you really expect everyone who ever writes any optimization or transformation over DFG IR to have to understand that DFG nodes may refer to each other in one of precisely two ways:

- Use edges.

- Super special StoreBarrier->storeNode() edges that exist solely for the purpose of avoiding doing a tiny bit of work in FixupPhase.

It's important that a compiler IR is *clean*.  This is how we can communicate it to each other, and how we can remember what the IR semantics are when we're stuck having to fix super subtle bugs.  The cleaner the IR, the easier it is to reason about the compiler's correctness.  Adding a super special way for nodes to refer to each other should have a *damn good* justification, and I just don't see such a justification here.

[Mark Hahnenberg]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (In reply to comment #8)
> > > > (From update of attachment 219433 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=219433&action=review
> > > > 
> > > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1592
> > > > > +        Node* storeNode = node->storeNode();
> > > > 
> > > > I really *really* don't like this. At all. The store barrier should not point to the "store" node. Instead, you should only plant a store barrier in fix up if you know that the store node needs one. Thereafter, the store barrier node should always unconditionally mean that you have a store barrier. 
> > > > 
> > > > The way you've done it currently is super fragile and I don't think we want it.
> > > 
> > > Why exactly is it so fragile?
> > 
> > There are multiple places that not only have to know what a StoreBarrier is, but also, how StoreBarrier ought to behave depending on its storeNode().  That's fragile: imagine changing the meaning of one of the store nodes, or adding a new store node; you'll have to add it in all of the places that StoreBarrier gets handled.  Also imagine adding a new phase that tries to understand what various nodes do; now you'll have to make sure you take care of StoreBarrier.  You've already done this wrong in this patch: Clobberize appears to assume that StoreBarrier always reads/writes BarrierState even though StoreBarrier does *nothing* for some kinds of storeNode().
> > 
> > The worst part about this complexity that you've added is that it accomplishes nothing.  Why can't you just *not emit a StoreBarrier* in fixup phase if the store node doesn't actually do a barrier-able store?  Why do you have to have each phase that inspects a StoreBarrier have to again recompute whether or not the fixup phase should have inserted the StoreBarrier to begin with?
> > 
> > The whole point of the fixup phase is to minimize this kind of madness.  Just change fixup phase to not insert a StoreBarrier if it isn't needed, and then you'll be able to delete a bunch of the code in this patch.
> 
> But, broadly, I think you're taking the wrong approach to DFG IR.  Store barriers are a *simple* concept.  They shouldn't require adding a whole new kind of node->node edge to the DFG IR.  A new opcode (StoreBarrier) is understandable.  But a new way in which nodes refer to each other is definitely overkill for such a simple thing.
> 
> Do you really expect everyone who ever writes any optimization or transformation over DFG IR to have to understand that DFG nodes may refer to each other in one of precisely two ways:
> 
> - Use edges.
> 
> - Super special StoreBarrier->storeNode() edges that exist solely for the purpose of avoiding doing a tiny bit of work in FixupPhase.
> 
> It's important that a compiler IR is *clean*.  This is how we can communicate it to each other, and how we can remember what the IR semantics are when we're stuck having to fix super subtle bugs.  The cleaner the IR, the easier it is to reason about the compiler's correctness.  Adding a super special way for nodes to refer to each other should have a *damn good* justification, and I just don't see such a justification here.

This is why we have code reviews!

First of all, the storeNode wasn't intentionally used to avoid doing work in the fixup phase, and I'm not advocating for it over another approach. It's just how I coded it up, and it's easy to get rid of it. I agree that storeNode doesn't buy us much if anything. I was mainly using it for referring to the other node's op. I can replace the storeNode with just the op. 

Second, it seems to me that the fixup phase doesn't have enough information to decide ahead of time whether or not a particular store node needs or doesn't need a store barrier unless it takes a very conservative approach. So why not just add StoreBarriers along with each node that might need them?  Then we could use the abstract interpreter and the write barrier elision phase to prune away ones that we could prove we didn't need. I'll also add code to use the predictions for the child nodes to avoid unnecessarily inserting store barriers.

Finally, I think that inserting store barriers *only* during the fixup phase is also a fragile approach. For any further transformations after the fixup phase you then have to be careful not to insert a node that could cause a GC between the store and initial barrier. That's why I also add StoreBarriers wherever we insert a new node that requires a barrier (e.g. in the constant folding phase). Again, it's simple: if you do a store then you insert a barrier node. Let somebody else figure out whether or not it was necessary.

[Filip Pizlo]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > (In reply to comment #8)
> > > > > (From update of attachment 219433 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=219433&action=review
> > > > > 
> > > > > > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:1592
> > > > > > +        Node* storeNode = node->storeNode();
> > > > > 
> > > > > I really *really* don't like this. At all. The store barrier should not point to the "store" node. Instead, you should only plant a store barrier in fix up if you know that the store node needs one. Thereafter, the store barrier node should always unconditionally mean that you have a store barrier. 
> > > > > 
> > > > > The way you've done it currently is super fragile and I don't think we want it.
> > > > 
> > > > Why exactly is it so fragile?
> > > 
> > > There are multiple places that not only have to know what a StoreBarrier is, but also, how StoreBarrier ought to behave depending on its storeNode().  That's fragile: imagine changing the meaning of one of the store nodes, or adding a new store node; you'll have to add it in all of the places that StoreBarrier gets handled.  Also imagine adding a new phase that tries to understand what various nodes do; now you'll have to make sure you take care of StoreBarrier.  You've already done this wrong in this patch: Clobberize appears to assume that StoreBarrier always reads/writes BarrierState even though StoreBarrier does *nothing* for some kinds of storeNode().
> > > 
> > > The worst part about this complexity that you've added is that it accomplishes nothing.  Why can't you just *not emit a StoreBarrier* in fixup phase if the store node doesn't actually do a barrier-able store?  Why do you have to have each phase that inspects a StoreBarrier have to again recompute whether or not the fixup phase should have inserted the StoreBarrier to begin with?
> > > 
> > > The whole point of the fixup phase is to minimize this kind of madness.  Just change fixup phase to not insert a StoreBarrier if it isn't needed, and then you'll be able to delete a bunch of the code in this patch.
> > 
> > But, broadly, I think you're taking the wrong approach to DFG IR.  Store barriers are a *simple* concept.  They shouldn't require adding a whole new kind of node->node edge to the DFG IR.  A new opcode (StoreBarrier) is understandable.  But a new way in which nodes refer to each other is definitely overkill for such a simple thing.
> > 
> > Do you really expect everyone who ever writes any optimization or transformation over DFG IR to have to understand that DFG nodes may refer to each other in one of precisely two ways:
> > 
> > - Use edges.
> > 
> > - Super special StoreBarrier->storeNode() edges that exist solely for the purpose of avoiding doing a tiny bit of work in FixupPhase.
> > 
> > It's important that a compiler IR is *clean*.  This is how we can communicate it to each other, and how we can remember what the IR semantics are when we're stuck having to fix super subtle bugs.  The cleaner the IR, the easier it is to reason about the compiler's correctness.  Adding a super special way for nodes to refer to each other should have a *damn good* justification, and I just don't see such a justification here.
> 
> This is why we have code reviews!
> 
> First of all, the storeNode wasn't intentionally used to avoid doing work in the fixup phase, and I'm not advocating for it over another approach. It's just how I coded it up, and it's easy to get rid of it. I agree that storeNode doesn't buy us much if anything. I was mainly using it for referring to the other node's op. I can replace the storeNode with just the op. 

What do you mean by replacing the storeNode with just the op?

I think that all possible solutions that are consistent with how ssa-like compiler IRs work would have the barrier not referring to the store in any way. 

> 
> Second, it seems to me that the fixup phase doesn't have enough information to decide ahead of time whether or not a particular store node needs or doesn't need a store barrier unless it takes a very conservative approach. So why not just add StoreBarriers along with each node that might need them?  Then we could use the abstract interpreter and the write barrier elision phase to prune away ones that we could prove we didn't need. I'll also add code to use the predictions for the child nodes to avoid unnecessarily inserting store barriers.

I could see this approach working: always insert store barriers and remove them when you realize that the predicted types are something you wouldn't barrier. It just seems like this could lead to dumbness. Remember after you insert the StoreBarrier you necessarily forget what you inserted it for. (You do this because anything else would be insane. And to appease the compiler gods who frown on such shenanigans.) So now say you have some node that is storing an int32 into a typed array. The store barrier's value child will be predicted int32. Should the store barrier speculate int32 or just NonCell?  In this case it should speculate int32. But what about the case where you're storing a double into a typed array? Here it's clearly bad if the store barrier speculates int32. It's also hugely inefficient if it speculates NonCell, since NonCell doesn't prove double (it proves int or double or Boolean or null or undefined, god I love this language!). See how that's rapidly descending into madness?  Otoh you can just observe, during fix up, that you're storing to a typed array and just not insert a store barrier to begin with!  Bang problem solved. You can of course still do other optimizations and reasoning, but fixup's existing knowledge is likely to prove useful: it'll prevent you from having to worry about large class of "obviously don't need a barrier" cases. 

Broadly, the fix up phase is exactly the right place to make the bulk of these decisions. That's the place where we know that we are speculating in a way that would result in a barrier. The typed array case is an example of this but there are probably others. 

The CFA would be conservative here since that doesn't add speculations and it's job is to preserve any speculations that had already been inserted. So for example if you inserted a store barrier that speculated int32 and then proved that the barrier is unneeded for unrelated reasons then you'd still need to keep the int32 check. 

It's ok to have the CFA and folder remove store barriers as an additional optimization, but I would y rely solely on CFA. 

> 
> Finally, I think that inserting store barriers *only* during the fixup phase is also a fragile approach. For any further transformations after the fixup phase you then have to be careful not to insert a node that could cause a GC between the store and initial barrier. That's why I also add StoreBarriers wherever we insert a new node that requires a barrier (e.g. in the constant folding phase). Again, it's simple: if you do a store then you insert a barrier node. Let somebody else figure out whether or not it was necessary.

This will never happen. It's incorrect for the compiler's post-fix up passes to make the code do things it wasn't doing before. If you sort of squint carefully you'll note that each phase essentially removes things. The closest thing to adding things is hoisting, but we don't hoist stores so store barriers don't come into play. 

Anyway. The whole point of fix up phase is to put things like this into that phase.

[Mark Hahnenberg]

> What do you mean by replacing the storeNode with just the op?
> 
> I think that all possible solutions that are consistent with how ssa-like compiler IRs work would have the barrier not referring to the store in any way. 

I just mean a field of type NodeType. Depending on the node it was inserted for, a StoreBarrier can do slightly different things (e.g. what if the original node didn't have a "value" child). I was just using the NodeType as a proxy for "this barrier has a base and a value, this barrier just has a base, this barrier uses the global object as its base, etc." To be clear, the stored NodeType would be immutable from the time of the barrier's creation, meaning there would be no direct link between the store node and the barrier node.

Alternatively I could introduce a different enum that represented each of these unique kinds of barrier, but I figured the ops themselves were good enough. 

> It's ok to have the CFA and folder remove store barriers as an additional optimization, but I would y rely solely on CFA. 
> 

Did you mean "I wouldn't rely solely..."?

> Anyway. The whole point of fix up phase is to put things like this into that phase.

After moving the checking of ArrayMode into fixup, I see what you mean by it moving a lot of code there from a bunch of other places.

[Filip Pizlo]

(In reply to comment #14)
> > What do you mean by replacing the storeNode with just the op?
> > 
> > I think that all possible solutions that are consistent with how ssa-like compiler IRs work would have the barrier not referring to the store in any way. 
> 
> I just mean a field of type NodeType.

Oh, OK - in that case carry on!

> Depending on the node it was inserted for, a StoreBarrier can do slightly different things (e.g. what if the original node didn't have a "value" child). I was just using the NodeType as a proxy for "this barrier has a base and a value, this barrier just has a base, this barrier uses the global object as its base, etc." 

I think you can do this as one of two ways, and I think both fit with existing DFG idioms.

1) Have two StoreBarrier nodes.  One I would call StoreBarrier, and the other I would call ConditionalStoreBarrier.  StoreBarrier would just take a base and nothing else and would imply always running a barrier on that object.  ConditionalStoreBarrier would also take a value and would only execute a barrier on the base if the value had the appropriate type or whatever.  I would handle the global object by simply planting a WeakJSConstant for the global object and using the other node types.

2) Have one StoreBarrier node, and make it behave like a ConditionalStoreBarrier if !!child2().

> To be clear, the stored NodeType would be immutable from the time of the barrier's creation, meaning there would be no direct link between the store node and the barrier node.

Yay!

> 
> Alternatively I could introduce a different enum that represented each of these unique kinds of barrier, but I figured the ops themselves were good enough.

I sometimes like to do that, like for the GetByVal.  But it's a really vicious trade-off.  For example, with the ByVal nodes and their ArrayMode, what often happens is at some point I'll have a brainfart and forget about all of the different ways that ByVal could behave and end up implementing a case for GetByVal in some phase that blindly assumes too much.  In my experience this kind of brainfarting is less likely when you have separate node types for all of the possible behaviors.  But, with GetByVal I currently prefer the ArrayMode approach just because there are *so darn many* array modes and there are enough places in the compiler where you just don't really care about the array mode.

So, I'm not opposed to having a separate enum, in principle.  But here, I think that the global object case can be expressed by just passing a WeakJSConstant as a base; the presence-or-lack of a value could just be expressed by either having a value child or not having one.

> 
> > It's ok to have the CFA and folder remove store barriers as an additional optimization, but I would y rely solely on CFA. 
> > 
> 
> Did you mean "I wouldn't rely solely..."?

Yup!

> 
> > Anyway. The whole point of fix up phase is to put things like this into that phase.
> 
> After moving the checking of ArrayMode into fixup, I see what you mean by it moving a lot of code there from a bunch of other places.

[Mark Hahnenberg]

Created attachment 219557 [details]
Patch

[Build Bot]

Comment on attachment 219557 [details]
Patch

Attachment 219557 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/46118079

[Mark Hahnenberg]

Created attachment 219559 [details]
Patch

[Filip Pizlo]

Comment on attachment 219559 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGAbstractInterpreter.h:155
> +    template <typename T>
> +    bool isKnownNotCell(T node)
> +    {
> +        return !(forNode(node).m_type & SpecCell);
> +    }
> +

I have a feeling there is already an idiom for this:

!m_interpreter.needsTypeCheck(node, ~SpecCell)

> Source/JavaScriptCore/dfg/DFGFixupPhase.cpp:1601
> +    void fixupHeapStore(Node* node)
> +    {
> +        Node* barrierNode = 0;
> +        switch (node->op()) {
> +        case ArrayPush: {
> +            switch (node->arrayMode().type()) {
> +            case Array::Contiguous:
> +            case Array::ArrayStorage: {
> +                barrierNode = m_graph.addNode(SpecNone, ConditionalStoreBarrier, m_currentNode->codeOrigin, 
> +                    Edge(node->child1().node(), KnownCellUse), Edge(node->child2().node(), UntypedUse));
> +            }
> +
> +            default:
> +                return;
> +            }
> +            break;
> +        }
> +
> +        case PutById:
> +        case PutByIdDirect: {
> +            barrierNode = m_graph.addNode(SpecNone, ConditionalStoreBarrier, m_currentNode->codeOrigin, 
> +                Edge(node->child1().node(), CellUse), Edge(node->child2().node(), UntypedUse));
> +            break;
> +        }
> +
> +        // The barrier must go after these nodes because they allocate their new property storage first and then do the store.
> +        case AllocatePropertyStorage:
> +        case ReallocatePropertyStorage: {
> +            barrierNode = m_graph.addNode(SpecNone, StoreBarrier, m_currentNode->codeOrigin, 
> +                Edge(node->child1().node(), KnownCellUse));
> +            fixupNode(barrierNode);
> +            m_insertionSet.insert(m_indexInBlock + 1, barrierNode);
> +            return;
> +        }
> +
> +        case PutStructure: {
> +            barrierNode = m_graph.addNode(SpecNone, StoreBarrier, m_currentNode->codeOrigin, 
> +                Edge(node->child1().node(), KnownCellUse));
> +            break;
> +        }
> +
> +        case PutGlobalVar: {
> +            Node* globalObjectNode = m_insertionSet.insertNode(m_indexInBlock, SpecNone, WeakJSConstant, node->codeOrigin, OpInfo(m_graph.globalObjectFor(node->codeOrigin)));
> +            barrierNode = m_graph.addNode(SpecNone, ConditionalStoreBarrier, m_currentNode->codeOrigin, 
> +                Edge(globalObjectNode, KnownCellUse), Edge(node->child1().node(), UntypedUse));
> +            break;
> +        }
> +
> +        case TearOffActivation: {
> +            barrierNode = m_graph.addNode(SpecNone, StoreBarrierWithNullCheck, m_currentNode->codeOrigin, 
> +                Edge(node->child1().node(), UntypedUse));
> +            break;
> +        }
> +
> +        case PutByOffset: {
> +            barrierNode = m_graph.addNode(SpecNone, ConditionalStoreBarrier, m_currentNode->codeOrigin, 
> +                Edge(node->child2().node(), KnownCellUse), Edge(node->child3().node(), UntypedUse));
> +            break;
> +        }
> +
> +        case PutClosureVar: {
> +            barrierNode = m_graph.addNode(SpecNone, ConditionalStoreBarrier, m_currentNode->codeOrigin, 
> +                Edge(node->child1().node(), KnownCellUse), Edge(node->child3().node(), UntypedUse));
> +            break;
> +        }
> +

Seems like it would be better to have a insertBarrier() helper that did insertNode/fixupNode.  That way you won't have to have this nested switch on NodeType.

[Filip Pizlo]

Comment on attachment 219559 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGStoreBarrierElisionPhase.cpp:49
> +    bool couldCauseGC(Node* node)

All of these methods should be private.  That's just how the other phases are.

And put the run() method at the top.

> Source/JavaScriptCore/dfg/DFGStoreBarrierElisionPhase.cpp:97
> +    void clearVariableBarriers()
> +    {
> +        for (unsigned i = 0; i < m_graph.m_variableAccessData.size(); ++i)
> +            m_graph.m_variableAccessData[i].checkAndSetNeedsBarrier(true);
> +    }

Kill the variable access data stuff.

> Source/JavaScriptCore/dfg/DFGStoreBarrierElisionPhase.cpp:152
> +    bool handleLocal(HashSet<Node*>& dontNeedBarriers, Node* node)
> +    {
> +        ASSERT(node->hasLocal(m_graph));
> +        switch (node->op()) {
> +        case GetLocal: {
> +            if (!node->variableAccessData()->needsBarrier())
> +                dontNeedBarriers.add(node);
> +            return false;
> +        }
> +
> +        case MovHint:
> +        case MovHintAndCheck:
> +        case SetLocal: {
> +            Node* child = node->child1().node();
> +            VariableAccessData* variable = node->variableAccessData();
> +            if (dontNeedBarriers.contains(child))
> +                return variable->checkAndSetNeedsBarrier(false);
> +            return variable->checkAndSetNeedsBarrier(true);
> +        }
> +
> +        default:
> +            return false;
> +        }
> +    }

Kill

> Source/JavaScriptCore/dfg/DFGStoreBarrierElisionPhase.cpp:195
> +    HashMap<BasicBlock*, HashSet<Node*>> m_nodeStateAtTail;

Kill this!

[Mark Hahnenberg]

Created attachment 219570 [details]
Patch

[Filip Pizlo]

Comment on attachment 219570 [details]
Patch

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

> Source/JavaScriptCore/dfg/DFGStoreBarrierElisionPhase.cpp:114
> +        if (node->op() != StoreBarrier)
> +            return;

Other kinds of barriers?

[Mark Hahnenberg]

Committed r160796: <http://trac.webkit.org/changeset/160796>

[Mark Hahnenberg]

Reopening to attach new patch.

[Mark Hahnenberg]

Created attachment 219585 [details]
Patch

[Mark Hahnenberg]

Command line flub, ignore this patch.