Create a super rough prototype of B3

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Filip Pizlo]

Yay.

[Filip Pizlo]

Created attachment 263377 [details]
it's a start

[Filip Pizlo]

This can land once it has enough things to do an end-to-end compile of the empty procedure.

[Filip Pizlo]

I just got an idea about how to handle constants, and wanted to write it down.

Here are our options for how to handle constants, assuming that B3IR places constant values inside control flow and then LICMs and GVNs them.

1) Let the register allocator treat Tmps that happened to come into existence because of constants as just normal Tmps.  They may get spilled.  After register allocation, run constant propagation through stores and loads to stack slots.  This will effectively cause rematerialization.

2) Have LowerB3ToAir choose registers for the tag constants, and sink all other constants to their blocks.

3) Have a late B3IR phase that sinks constants.  I'd prefer an approach that special-cases a hard limit on hoistable constants and then marks those Values that contain hoisted constants as "must allocate" for the register allocator.  All other constants are sunk to the blocks that use them.

4) Have floating constants in Air and require the register allocator in figure it out.  This would make sense if the register allocator did live range splitting.  I'm not sure I like this approach.

5) Here's a possible approach that I just thought of, and I don't know yet if it makes sense.  Do maximal constant sinking.  Then, have an Air constant propagator that eliminates instructions that materialize a constant if that constant is already available in some register.

6) Similar to above, but a different way of thinking about it. Let constants float in Air.  Register allocator treats this like a use of a register at the point of use of the constant.  Like if we have:

store $hugeConst, (%temp)

Then the register allocator sees it as if it was:

mov $hugeConst, %constTemp
store %constTemp, (%temp)

It assigns registers to the %constTemp's - i.e. to the "edges" between operations and the constants they use. Then, after doing register allocation, we can construct the constant materializations via forward flow. At each use of a %constTemp, we create the most efficient instruction possible by looking at what kinds of registers we have in constants. Any constant materialization - either one that we created or one that happened to be in the code already - causes us to know that henceforth, that register has that constant. We can abstractly interpret the Air code and clobber registers appropriately. So, we can optimize the %constTemp use to an add instruction if there is a nearby constant. Or, because the %constTemp's register use is only valid at the instant in time at the start of execution of the instruction that uses it, we can ignore the register allocator's choice of register and replace it with the use of a register that already has that constant.  For example, we may have:

1: store $hugeConst, (%temp)
2: store $hugeConst, (%temp2)

This implicitly has %constTemp1 and %constTemp2.  The register allocator may assign some registers to these.  Lets say that it does %constTemp1=%rax and %constTemp2=%rsi.  Then, the forward algorithm does this:

For instruction #1: It observes that no register currently contains any constant, so it prepends a “mov $hugeConst, %rax” instruction.  That’s because the register allocator told us that %rax is an appropriate place to put the constant. Then, it’ll transform instruction #1 to use %rax instead of $hugeConst. It will record that %rax contains $hugeConst.

For instruction #2: It observes that %rax already contains $hugeConst. Since the %constTemp2=%rsi register assignment was just a suggestion in case no register contained the constant yet, we just transform instruction #2 to use %rax instead of $hugeConst.

Note that since register allocators are usually deterministic, both %constTemp1 and %constTemp2 would probably get the same register.

The only missing piece here is hoisting constants out of loops. We could have LICM for these constant materializations. If a loop does not contain any other assignment to the register used for a constant materialization and does not clobber the register, then we can move the constant materialization out of the loop.

I think that probably the best solution will be a hybrid of (2) and (6). At least one of the tag values is super critical and we should just pin a callee-save register to it. We could even have a configuration parameter called "number of critical constants". It will pick that number of hottest constants in the program and materialize them in the prologue.

[Filip Pizlo]

Created attachment 263397 [details]
more

[Filip Pizlo]

Created attachment 263403 [details]
it's growing

[Filip Pizlo]

Created attachment 263404 [details]
just a bit more

[Filip Pizlo]

Created attachment 263410 [details]
no big deal, fleshing out the IR

[Filip Pizlo]

Created attachment 263425 [details]
it has stackmaps and patchpoints, sort of

[Mark Lam]

Pardon my ignorance, but what does B3 stand for?

[Filip Pizlo]

Created attachment 263427 [details]
B3 instructions are fleshed out

[Filip Pizlo]

Created attachment 263432 [details]
started playing with instruction selection

[Filip Pizlo]

Been thinking about pattern matching some more.  We want some way of ensuring that if we hoist a BaseIndex expression out of a loop, then inside the loop we don't match it - we use the generated variable.

We could make this work by having the pattern matcher refuse to match a pattern around something that already has a low-level variable.

[Filip Pizlo]

Created attachment 263434 [details]
writing a pattern matcher

[Filip Pizlo]

Woohoo!  I have a recursive pattern match generator.  It takes this:

matcher ComparisonMatcher

TestLoadNotZero1 = And(Load(address), right)
TestLoadNotZero2 = And(left, Load(address))
TestNotZero = And(left, right)

TestLoadZero1 = Equal(And(Load(address), right), $0)
TestLoadZero2 = Equal(And(left, Load(address)), $0)
TestZero = Equal(And(left, right), $0)

LoadEqual1 = Equal(Load(address), right)
LoadEqual2 = Equal(left, Load(address))
Not = Equal(value, $0)
Equal = Equal(left, right)

And turns it into this:

// Generated by generate_pattern_matcher.rb from Source/JavaScriptCore/b3/B3ComparisonMatcher.patterns -- do not edit!
#ifndef B3ComparisonMatcher_h
#define B3ComparisonMatcher_h
#include "B3Value.h"
namespace JSC { namespace B3 {
template<typename Adaptor>
void runComparisonMatcher(Value* value, Adaptor& adaptor)
{
    switch (value->opcode()) {
    case And:
        if (value->children.size() >= 2) {
            {
                Value* _tmp14 = value->child(0);
                switch (_tmp14->opcode()) {
                case Load:
                    if (_tmp14->children.size() >= 1) {
                        {
                            Value* _tmp15 = _tmp14->child(0);
                            {
                                Value* _tmp16 = value->child(1);
                                {
                                    Value* TestLoadNotZero1 = value;
                                    Value* _tmp1 = TestLoadNotZero1->child(0);
                                    Value* address = _tmp1->child(0);
                                    Value* right = TestLoadNotZero1->child(1);
                                    if (adaptor.acceptRoot(value)
                                        && adaptor.acceptInternals(_tmp1)
                                        && adaptor.acceptOperands(address, right)
                                        && adaptor.tryTestLoadNotZero1(address, right))
                                        return;
                                }
                            }
                        }
                    }
                    break;
                default:
                    break;
                }
                {
                    Value* _tmp17 = value->child(1);
                    switch (_tmp17->opcode()) {
                    case Load:
                        if (_tmp17->children.size() >= 1) {
                            {
                                Value* _tmp18 = _tmp17->child(0);
                                {
                                    Value* TestLoadNotZero2 = value;
                                    Value* left = TestLoadNotZero2->child(0);
                                    Value* _tmp2 = TestLoadNotZero2->child(1);
                                    Value* address = _tmp2->child(0);
                                    if (adaptor.acceptRoot(value)
                                        && adaptor.acceptInternals(_tmp2)
                                        && adaptor.acceptOperands(left, address)
                                        && adaptor.tryTestLoadNotZero2(left, address))
                                        return;
                                }
                            }
                        }
                        break;
                    default:
                        break;
                    }
                    {
                        Value* TestNotZero = value;
                        Value* left = TestNotZero->child(0);
                        Value* right = TestNotZero->child(1);
                        if (adaptor.acceptRoot(value)
                            && adaptor.acceptInternals()
                            && adaptor.acceptOperands(left, right)
                            && adaptor.tryTestNotZero(left, right))
                            return;
                    }
                }
            }
        }
        break;
    case Equal:
        if (value->children.size() >= 2) {
            {
                Value* _tmp19 = value->child(0);
                switch (_tmp19->opcode()) {
                case And:
                    if (_tmp19->children.size() >= 2) {
                        {
                            Value* _tmp20 = _tmp19->child(0);
                            switch (_tmp20->opcode()) {
                            case Load:
                                if (_tmp20->children.size() >= 1) {
                                    {
                                        Value* _tmp21 = _tmp20->child(0);
                                        {
                                            Value* _tmp22 = _tmp19->child(1);
                                            {
                                                Value* _tmp23 = value->child(1);
                                                {
                                                    Value* TestLoadZero1 = value;
                                                    Value* _tmp4 = TestLoadZero1->child(0);
                                                    Value* _tmp3 = _tmp4->child(0);
                                                    Value* address = _tmp3->child(0);
                                                    Value* right = _tmp4->child(1);
                                                    Value* _tmp5 = TestLoadZero1->child(1);
                                                    if (_tmp5->isInt(0)) {
                                                        if (adaptor.acceptRoot(value)
                                                            && adaptor.acceptInternals(_tmp4, _tmp3)
                                                            && adaptor.acceptOperands(address, right, _tmp5)
                                                            && adaptor.tryTestLoadZero1(address, right))
                                                            return;
                                                    }
                                                }
                                            }
                                        }
                                    }
                                }
                                break;
                            default:
                                break;
                            }
                            {
                                Value* _tmp24 = _tmp19->child(1);
                                switch (_tmp24->opcode()) {
                                case Load:
                                    if (_tmp24->children.size() >= 1) {
                                        {
                                            Value* _tmp25 = _tmp24->child(0);
                                            {
                                                Value* _tmp26 = value->child(1);
                                                {
                                                    Value* TestLoadZero2 = value;
                                                    Value* _tmp7 = TestLoadZero2->child(0);
                                                    Value* left = _tmp7->child(0);
                                                    Value* _tmp6 = _tmp7->child(1);
                                                    Value* address = _tmp6->child(0);
                                                    Value* _tmp8 = TestLoadZero2->child(1);
                                                    if (_tmp8->isInt(0)) {
                                                        if (adaptor.acceptRoot(value)
                                                            && adaptor.acceptInternals(_tmp7, _tmp6)
                                                            && adaptor.acceptOperands(left, address, _tmp8)
                                                            && adaptor.tryTestLoadZero2(left, address))
                                                            return;
                                                    }
                                                }
                                            }
                                        }
                                    }
                                    break;
                                default:
                                    break;
                                }
                                {
                                    Value* _tmp27 = value->child(1);
                                    {
                                        Value* TestZero = value;
                                        Value* _tmp9 = TestZero->child(0);
                                        Value* left = _tmp9->child(0);
                                        Value* right = _tmp9->child(1);
                                        Value* _tmp10 = TestZero->child(1);
                                        if (_tmp10->isInt(0)) {
                                            if (adaptor.acceptRoot(value)
                                                && adaptor.acceptInternals(_tmp9)
                                                && adaptor.acceptOperands(left, right, _tmp10)
                                                && adaptor.tryTestZero(left, right))
                                                return;
                                        }
                                    }
                                }
                            }
                        }
                    }
                    break;
                case Load:
                    if (_tmp19->children.size() >= 1) {
                        {
                            Value* _tmp28 = _tmp19->child(0);
                            {
                                Value* _tmp29 = value->child(1);
                                {
                                    Value* LoadEqual1 = value;
                                    Value* _tmp11 = LoadEqual1->child(0);
                                    Value* address = _tmp11->child(0);
                                    Value* right = LoadEqual1->child(1);
                                    if (adaptor.acceptRoot(value)
                                        && adaptor.acceptInternals(_tmp11)
                                        && adaptor.acceptOperands(address, right)
                                        && adaptor.tryLoadEqual1(address, right))
                                        return;
                                }
                            }
                        }
                    }
                    break;
                default:
                    break;
                }
                {
                    Value* _tmp30 = value->child(1);
                    switch (_tmp30->opcode()) {
                    case Load:
                        if (_tmp30->children.size() >= 1) {
                            {
                                Value* _tmp31 = _tmp30->child(0);
                                {
                                    Value* LoadEqual2 = value;
                                    Value* left = LoadEqual2->child(0);
                                    Value* _tmp12 = LoadEqual2->child(1);
                                    Value* address = _tmp12->child(0);
                                    if (adaptor.acceptRoot(value)
                                        && adaptor.acceptInternals(_tmp12)
                                        && adaptor.acceptOperands(left, address)
                                        && adaptor.tryLoadEqual2(left, address))
                                        return;
                                }
                            }
                        }
                        break;
                    default:
                        break;
                    }
                    {
                        Value* Not = value;
                        Value* value = Not->child(0);
                        Value* _tmp13 = Not->child(1);
                        if (_tmp13->isInt(0)) {
                            if (adaptor.acceptRoot(value)
                                && adaptor.acceptInternals()
                                && adaptor.acceptOperands(value, _tmp13)
                                && adaptor.tryNot(value))
                                return;
                        }
                    }
                    {
                        Value* Equal = value;
                        Value* left = Equal->child(0);
                        Value* right = Equal->child(1);
                        if (adaptor.acceptRoot(value)
                            && adaptor.acceptInternals()
                            && adaptor.acceptOperands(left, right)
                            && adaptor.tryEqual(left, right))
                            return;
                    }
                }
            }
        }
        break;
    default:
        break;
    }
    RELEASE_ASSERT_NOT_REACHED();
}
} } // namespace JSC::B3
#endif // B3ComparisonMatcher_h

[Filip Pizlo]

Created attachment 263436 [details]
and now, with a complete pattern match generator

Now we can go nuts with patterns.

[Filip Pizlo]

Created attachment 263528 [details]
started working on Air

This is pretty awesome - lots of Air functionality is just auto-generated from the AirOpcodes.opcodes spec.

[Filip Pizlo]

Created attachment 263529 [details]
removed some stuff

Removed my previous attempt at reflective code generation.  Before I started working on Air, I had added this DynamicAddress thing to the assembler.  I won't be using it, so this version of the patch removes it.

[Filip Pizlo]

Created attachment 263546 [details]
a little more

Added more things to Air.  I think that the IR scaffolding is complete.  Now we'll have to add all of the instructions.

[Filip Pizlo]

Created attachment 263549 [details]
a bit more

[Filip Pizlo]

Created attachment 263615 [details]
cool story for specials

[Filip Pizlo]

Created attachment 263623 [details]
just a few little things

Starting to glue Air's notion of Special to B3's notion of stackmap.

[Filip Pizlo]

I'm currently thinking that patterns like "Add(Load(address), value)" should not be listed in the patterns file but instead should arise out of C++ code.  I've been playing with pseudocode for how to say this.  Here's something I was just thinking about for a universal helper for emitting math ops.

Note that we need the opcode_generator to emit isValidForm() methods for this to be efficient.  I'm thinking that we could even templatize them like isValidForm<opcode>(...), and then have the appendOp also be templatized like appendOp<opcode, Commutative>(left, right).  That would be sooooper fast.

void appendInst(const Inst& inst)
{
    ASSERT(inst.isValidForm());
    m_block->appendInst(inst);
}

template<typename... Arguments>
void append(Opcode opcode, Arguments... arguments)
{
    appendInst(Inst(opcode, m_value, arguments...));
}

Arg addr(Value* value)
{
    // Makes sure that value can be made into an internal value for this pattern, and then can get
    // locked.
}

void appendOp(Air::Opcode opcode, Value* left, Value* right, Commutativity commutativity = NotCommutative)
{
    Tmp result = resultTmp();
    // Three-operand forms like:
    //     Op a, b, c
    // mean something like:
    //     c = a Op b
    if (isValidForm(opcode, Arg::Imm, Arg::Tmp, Arg::Tmp)) {
        if (commutativity == Commutative) {
            if (isImm(right)) {
                append(opcode, imm(right), tmp(left), result);
                return;
            }
        } else {
            // A non-commutative operation could have an immediate in left.
            if (isImm(left)) {
                append(opcode, imm(left), tmp(right), result);
                return;
            }
        }
    }
    if (isImm(right) && isValidForm(opcode, Arg::Tmp, Arg::Imm, Arg::Tmp)) {
        append(opcode, tmp(left), imm(right), result);
        return;
    }
    if (commutativity == Commutative && left->opcode() == Load) {
        Arg leftAddr = addr(left);
        if (isValidForm(opcode, leftAddr.kind(), Arg::Tmp)) {
            append(Move, tmp(right), result);
            append(opcode, leftAddr, result);
            return;
        }
    }
    append(Move, tmp(left), result);
    if (right->opcode() == Load) {
        Arg rightAddr = addr(right);
        if (isValidForm(opcode, rightAddr.kind(), Arg::Tmp)) {
            append(opcode, rightAddr, result);
            return;
        }
    }
    // Two-operand forms only work when right is an immediate, since we clobber left in place.
    if (isImm(right) && isValidForm(opcode, Arg::Imm, Arg::Tmp)) {
        append(imm(right), result);
        return;
    }

    append(tmp(right), result);
}

[Filip Pizlo]

Created attachment 263633 [details]
latest

I'm thinking about stackmaps so hard right now.

[Filip Pizlo]

Created attachment 263661 [details]
some more

[Filip Pizlo]

Created attachment 263739 [details]
even more

so much code

[Filip Pizlo]

Created attachment 263746 [details]
and now, with a maximal spiller

[Filip Pizlo]

Created attachment 263778 [details]
and now, with a stack allocator

[Filip Pizlo]

Created attachment 263845 [details]
air is done, maybe?

[Filip Pizlo]

Created attachment 263871 [details]
adding code layout support

[Filip Pizlo]

Created attachment 263878 [details]
started working on B3LowerToAir

I'm getting so close!

[Benjamin Poulain]

Comment on attachment 263845 [details]
air is done, maybe?

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

Quick comments:

> Source/JavaScriptCore/b3/B3BasicBlockUtils.h:36
> +bool addPredecessor(BasicBlock* block, BasicBlock* predecessor)

The arguments should be references here, to follow WebKit's new love for reference

> Source/JavaScriptCore/b3/B3BasicBlockUtils.h:48
> +bool removePredecessor(BasicBlock* block, BasicBlock *predecessor)

The "*" is on the wrong side on "BasicBlock *predecessor"

> Source/JavaScriptCore/b3/B3BasicBlockUtils.h:58
> +            result = true;
> +            // Out of caution, we keep going even though the predecessor list cannot have
> +            // duplicates.

This could help hiding programming mistakes.

What about?
    ASSERT_WITH_MESSAGE(!predecessors.contains(predecessor), "Any predecessor must only appear once per basic block adjacency list");
    return true;

> Source/JavaScriptCore/b3/air/AirBasicBlock.h:98
> +    BasicBlock(unsigned index)
> +    {

Don't forget 
    : m_index(index)

> Source/JavaScriptCore/b3/air/AirInst.h:85
> +        for (Arg arg : args)

"Arg" does not hold in a single register. Arg->Arg& here?

> Source/JavaScriptCore/b3/air/AirInstInlines.h:66
> +    return args[0].special()->extraClobberedRegs(*this);

Precondition:
    ASSERT_WITH_MESSAGE(hasSpecial(), "This operation only make sense for Specials.");

> Source/JavaScriptCore/b3/air/AirInstInlines.h:71
> +    args[0].special()->reportUsedRegisters(*this, usedRegisters);

ditto

> Source/JavaScriptCore/b3/air/AirLiveness.h:57
> +                if (!firstTime && localCalc.live() == m_liveAtHead[block])
> +                    continue;

This does not look right. You do not update the liveAtTail of the predecessors.

You should check how I modified the DFG Liveness analysis. I reduced the cost of predecessor update by making it depends on "localCalc.live() == m_liveAtHead[block]".

> Source/JavaScriptCore/b3/air/AirTmp.h:37
> +class Tmp {

I can live with the name, but I am not a fan of it.

> Source/JavaScriptCore/b3/air/AirTmp.h:71
> +    static Tmp forRawValue(unsigned index)

unsigned->int?

If possible, drop forRawValue(), that looks like a footgun.

> Source/JavaScriptCore/b3/air/AirTmp.h:80
> +    bool rawValue() const { return m_value; }

bool?

Can we drop this?

> Source/JavaScriptCore/b3/air/AirTmp.h:134
> +    unsigned gpTmpIndex() const
> +    {
> +        return decodeGPTmp(m_value);
> +    }
> +
> +    unsigned fpTmpIndex() const
> +    {
> +        return decodeFPTmp(m_value);
> +    }
> +
> +    unsigned tmpIndex() const

"tmp" here means unnamed/uncolored/unassigned.

Since the name of the class is already Tmp, this is a bit confusing.

[Filip Pizlo]

(In reply to comment #31)
> Comment on attachment 263845 [details]
> air is done, maybe?
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263845&action=review
> 
> Quick comments:
> 
> > Source/JavaScriptCore/b3/B3BasicBlockUtils.h:36
> > +bool addPredecessor(BasicBlock* block, BasicBlock* predecessor)
> 
> The arguments should be references here, to follow WebKit's new love for
> reference

I really don't want to do that.  I understand that WebKit has a love for references, but that love hasn't happened in JSC.  It's not how we've done things in the DFG for example.  In DFG, a Node and BasicBlock are referenced with * always, and that is a convention we have held to quite strictly.  It makes sense to me to designate certain classes as "always *".

The classes where it makes most sense to always use * are those where identity has semantics and it's common to say things like "a == b" when given "T*a, *b".  This is definitely the case with Node/BasicBlock in DFG, Value/BasicBlock in B3, and BasicBlock/Special/StackSlot in Air.

> 
> > Source/JavaScriptCore/b3/B3BasicBlockUtils.h:48
> > +bool removePredecessor(BasicBlock* block, BasicBlock *predecessor)
> 
> The "*" is on the wrong side on "BasicBlock *predecessor"

Oops, fixed.

> 
> > Source/JavaScriptCore/b3/B3BasicBlockUtils.h:58
> > +            result = true;
> > +            // Out of caution, we keep going even though the predecessor list cannot have
> > +            // duplicates.
> 
> This could help hiding programming mistakes.
> 
> What about?
>     ASSERT_WITH_MESSAGE(!predecessors.contains(predecessor), "Any
> predecessor must only appear once per basic block adjacency list");
>     return true;

Good idea, I changed it.

> 
> > Source/JavaScriptCore/b3/air/AirBasicBlock.h:98
> > +    BasicBlock(unsigned index)
> > +    {
> 
> Don't forget 
>     : m_index(index)

Done.

> 
> > Source/JavaScriptCore/b3/air/AirInst.h:85
> > +        for (Arg arg : args)
> 
> "Arg" does not hold in a single register. Arg->Arg& here?

OMG that was a huge error, since it's legal for the Functor to take Tmp& and modify the Tmp's. The lack of & would have meant that it wouldn't have actually modified the Inst's Tmp's!

Fixed.

> 
> > Source/JavaScriptCore/b3/air/AirInstInlines.h:66
> > +    return args[0].special()->extraClobberedRegs(*this);
> 
> Precondition:
>     ASSERT_WITH_MESSAGE(hasSpecial(), "This operation only make sense for
> Specials.");

I added an ASSERT, though we already would have gotten the assert implicitly inside Arg::special() (if size >= 1) or Vector::size() (if size == 0).  I've been restraining myself a little bit when it comes to redundant asserts, since we now have some places where the number of asserts is so large that it's hard to find the real code!

> 
> > Source/JavaScriptCore/b3/air/AirInstInlines.h:71
> > +    args[0].special()->reportUsedRegisters(*this, usedRegisters);
> 
> ditto

Ditto, I added the assert.

> 
> > Source/JavaScriptCore/b3/air/AirLiveness.h:57
> > +                if (!firstTime && localCalc.live() == m_liveAtHead[block])
> > +                    continue;
> 
> This does not look right. You do not update the liveAtTail of the
> predecessors.
> 
> You should check how I modified the DFG Liveness analysis. I reduced the
> cost of predecessor update by making it depends on "localCalc.live() ==
> m_liveAtHead[block]".

As you were writing your feedback, I had noticed this as well and fixed it. The loop body now does:

                BasicBlock* block = code.at(blockIndex);
                LocalCalc localCalc(*this, block);
                for (size_t instIndex = block->size(); instIndex--;)
                    localCalc.execute(block->at(instIndex));
                bool firstTime = seen.add(block);
                if (!firstTime && localCalc.live() == m_liveAtHead[block])
                    continue;
                changed = true;
                for (BasicBlock* predecessor : block->predecessors()) {
                    m_liveAtTail[predecessor].add(
                        localCalc.live().begin(), localCalc.live().end());
                }
                m_liveAtHead[block] = localCalc.takeLive();

I believe this sort of matches what you did in the DFG.

> 
> > Source/JavaScriptCore/b3/air/AirTmp.h:37
> > +class Tmp {
> 
> I can live with the name, but I am not a fan of it.

Neither am I, other than I'm expecting to have to write a lot of expressions that say "Tmp".  Also, a big goal with naming Arg::Kind's was to keep the AirOpcode.opcodes file terse. Hence "Tmp", "Imm", "Addr", and "Index" rather than their more verbose names.

> 
> > Source/JavaScriptCore/b3/air/AirTmp.h:71
> > +    static Tmp forRawValue(unsigned index)
> 
> unsigned->int?
> 
> If possible, drop forRawValue(), that looks like a footgun.

I removed it.  I thought I would make Arg be a proper union, in which case I'd need to extract a union-compatible thing from Tmp.  But, I ended up not doing it.

> 
> > Source/JavaScriptCore/b3/air/AirTmp.h:80
> > +    bool rawValue() const { return m_value; }
> 
> bool?
> 
> Can we drop this?

Removed.

> 
> > Source/JavaScriptCore/b3/air/AirTmp.h:134
> > +    unsigned gpTmpIndex() const
> > +    {
> > +        return decodeGPTmp(m_value);
> > +    }
> > +
> > +    unsigned fpTmpIndex() const
> > +    {
> > +        return decodeFPTmp(m_value);
> > +    }
> > +
> > +    unsigned tmpIndex() const
> 
> "tmp" here means unnamed/uncolored/unassigned.
> 
> Since the name of the class is already Tmp, this is a bit confusing.

Yeah.  I don't know what else to call it.  On the one hand, a Reg is a Tmp.  But on the other hand, there is a set of Tmps that are not Regs.

I found that this is easier to justify when I just say that "some Tmps have indices, and some don't".  I documented this by adding this method:

    bool hasTmpIndex() const
    {
        return !isReg();
    }

[Filip Pizlo]

Created attachment 263889 [details]
lots of progress on B3->Air lowering

[Filip Pizlo]

Created attachment 263900 [details]
I think that the B3->Air lowering is "done"

Done in the sense that it's good to commit.

[Saam Barati]

Comment on attachment 263900 [details]
I think that the B3->Air lowering is "done"

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

This is super interesting. I have a lot of questions but I'll make a second pass when I'm not on my phone. 
I have some fly by comments.

> Source/JavaScriptCore/b3/B3BasicBlockUtils.h:55
> +            // Out of caution, we keep going even though the predecessor list cannot have

This comment looks out of date now that you've added the assert.

> Source/JavaScriptCore/b3/B3ControlValue.h:48
> +            // swtichValue->as<ControlValue>() to return non-null.

Typo

> Source/JavaScriptCore/b3/B3ControlValue.h:94
> +    // Use this for Jump.

Maybe these could be static functions that call the proper constructors so that the call site is more descriptive?

> Source/JavaScriptCore/b3/B3IndexMap.h:35
> +template<typename Key, typename Value>

Does this need to be templatized over Key?
Or maybe you were going to add a method for "Value& get(Key);"?

> Source/JavaScriptCore/b3/B3IndexMap.h:59
> +    Vector<Value> m_vector;

Maybe it's worth giving this some inline storage?

> Source/JavaScriptCore/b3/B3IndexSet.h:54
> +            return nullptr;

I would make this "return false"

> Source/JavaScriptCore/b3/B3Opcode.h:71
> +    ChillDiv, // doesn't trap ever, behaves like JS (x/y)|0.

Nice

> Source/JavaScriptCore/b3/B3Opcode.h:136
> +    // It's legal to request that a stackmap argument is in some register and it's legal to request

Would it be possible to request it to be part of a certain set of constraints?
Like: must be a callee save or on the stack?

> Source/JavaScriptCore/b3/B3Opcode.h:166
> +    // full branch fusion in the instruction selector. A true value jumps to the generator's slow

What is branch fusion?

> Source/JavaScriptCore/b3/B3StackSlotKind.h:32
> +    // A locked stack slot needs to be keep disjoint from all others because we intend to escape

"be keep" => "be"

> Source/JavaScriptCore/b3/B3Stackmap.h:116
> +    void* m_opaque { nullptr };

Maybe a better name for this is "m_identifier" or "m_uniqueIdentifier"?

> Source/JavaScriptCore/b3/B3Type.h:52
> +    if (sizeof(void*) == 4)

Are we going to support 32-bit?

> Source/JavaScriptCore/b3/B3ValueInlines.h:46
> +template>typename T>

">" => "<"

[Benjamin Poulain]

Comment on attachment 263900 [details]
I think that the B3->Air lowering is "done"

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

Some more comments:

> Source/JavaScriptCore/b3/air/AirValidate.cpp:39
> +        , m_when(when)

Where is this one defined?

> Source/JavaScriptCore/jit/Reg.h:210
> +    static AllRegsIterable all() const { return AllRegsIterable(); }

The whole AllRegsIterable code seems unused, I don't see any call to all().

> Source/JavaScriptCore/jit/RegisterAtOffsetList.h:70
> +    template<typename Functor>
> +    void forEach(const Functor& functor) const

It looks like every use of this would be made cleaner with an iterator. Same for the next one.

Can you add a begin() + end() instead?

> Source/WTF/wtf/BitVector.h:323
> -    
> +

Let's revert this.

> Source/WTF/wtf/ScopedLambda.h:45
> +template<typename FunctionType> class ScopedLambda;
> +template<typename ResultType, typename... ArgumentTypes>
> +class ScopedLambda<ResultType (ArgumentTypes...)> {

I have never seen this syntax.

Is the initial declaration
    template<typename FunctionType> class ScopedLambda;
necessary?

> Source/WTF/wtf/ScopedLambda.h:55
> +    ~ScopedLambda()
> +    {
> +    }

You can remove the explicit destructor.

> Source/WTF/wtf/ScopedLambda.h:63
> +    ResultType (*impl)(void* arg, ArgumentTypes&&...) m_impl;

I am not sure you need the name "impl" for a type declaration.

[Filip Pizlo]

(In reply to comment #36)
> Comment on attachment 263900 [details]
> I think that the B3->Air lowering is "done"
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263900&action=review
> 
> Some more comments:
> 
> > Source/JavaScriptCore/b3/air/AirValidate.cpp:39
> > +        , m_when(when)
> 
> Where is this one defined?

Fixed!

> 
> > Source/JavaScriptCore/jit/Reg.h:210
> > +    static AllRegsIterable all() const { return AllRegsIterable(); }
> 
> The whole AllRegsIterable code seems unused, I don't see any call to all().

True. :-/  I added this because I saw too many loops like:

    for (Reg reg = Reg::first(); reg <= Reg::last(); reg = reg.next()) {

And I was *about* to write one in AirRegisterPriority.cpp. That's why I added this iterable.

What do you think of this patch fixing RegisterAtOffsetList to use this?

> 
> > Source/JavaScriptCore/jit/RegisterAtOffsetList.h:70
> > +    template<typename Functor>
> > +    void forEach(const Functor& functor) const
> 
> It looks like every use of this would be made cleaner with an iterator. Same
> for the next one.
> 
> Can you add a begin() + end() instead?

Sure, I changed this to use iterators.

> 
> > Source/WTF/wtf/BitVector.h:323
> > -    
> > +
> 
> Let's revert this.

Done.

> 
> > Source/WTF/wtf/ScopedLambda.h:45
> > +template<typename FunctionType> class ScopedLambda;
> > +template<typename ResultType, typename... ArgumentTypes>
> > +class ScopedLambda<ResultType (ArgumentTypes...)> {
> 
> I have never seen this syntax.
> 
> Is the initial declaration
>     template<typename FunctionType> class ScopedLambda;
> necessary?

Yeah, it is.  Something that helps understand this is that it *could* be rewritten as:

template<typename> class ScopedLambda;

Here's what this tells the C++ compiler:
- The valid syntax for instantiating the ScopedLambda template involves exactly one type argument.
- Any future declarations of a ScopedLambda template are specializations of ScopedLambda<T>. The compiler will try to pick the specialization where T matches that specialization's pattern.

A classic use of this syntax is something like:

template<typename> class PtrExtractor;
template<typename T>
class PtrExtractor<T*> { typedef T Type; }

Then if you say "PtrExtractor<char*>" then the compiler will notice the initial declaration, and then look for specializations. Because "char*" matches the "T*" pattern, we get PtrExtractor<char*>::Type == char.

This same kind of pattern matching works when the specialization has more type parameters than the original declaration. This could work for example:

template<typename> class PairExtractor;
template<typename T, typename U>
class PairExtractor<std::pair<T, U>> { typedef T FirstType; typedef U SecondType; };

Then you'd have PairExtractor<std::pair<char, int>>::FirstType == char, for example.

This ScopedLambda thing does the same thing, except to extract return value and argument types.  It took me a while to figure this out, but I already know that this pattern works - see wtf/SharedTask.h.  It's also what std::function does.

> 
> > Source/WTF/wtf/ScopedLambda.h:55
> > +    ~ScopedLambda()
> > +    {
> > +    }
> 
> You can remove the explicit destructor.

Lol, of course.

> 
> > Source/WTF/wtf/ScopedLambda.h:63
> > +    ResultType (*impl)(void* arg, ArgumentTypes&&...) m_impl;
> 
> I am not sure you need the name "impl" for a type declaration.

OMG, this should be:

ResultType (*m_impl)(void* arg, ArgumentTypes&&...)

[Filip Pizlo]

(In reply to comment #35)
> Comment on attachment 263900 [details]
> I think that the B3->Air lowering is "done"
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263900&action=review
> 
> This is super interesting. I have a lot of questions but I'll make a second
> pass when I'm not on my phone. 
> I have some fly by comments.
> 
> > Source/JavaScriptCore/b3/B3BasicBlockUtils.h:55
> > +            // Out of caution, we keep going even though the predecessor list cannot have
> 
> This comment looks out of date now that you've added the assert.

Removed!

> 
> > Source/JavaScriptCore/b3/B3ControlValue.h:48
> > +            // swtichValue->as<ControlValue>() to return non-null.
> 
> Typo

Fixed!

> 
> > Source/JavaScriptCore/b3/B3ControlValue.h:94
> > +    // Use this for Jump.
> 
> Maybe these could be static functions that call the proper constructors so
> that the call site is more descriptive?

No, because B3::Procedure has to instantiate values. B3::Procedure is the thing that supplies the "index" parameter. All B3 values will be instantiated using:

procedure.add<ControlValue>(Jump, Origin(...), target);

Though often times, you'll do something like:

insertionSet.insert<ControlValue>(indexInBlock, Jump, Origin(...), target);

or:

block->append<ControlValue>(procedure, Jump, Origin(...), target);

> 
> > Source/JavaScriptCore/b3/B3IndexMap.h:35
> > +template<typename Key, typename Value>
> 
> Does this need to be templatized over Key?

Yes.

> Or maybe you were going to add a method for "Value& get(Key);"?

Lol, yeah.  Actually I meant to add this:

    Value& operator[](Key* key)
    {
        return m_vector[key->index()];
    }

> 
> > Source/JavaScriptCore/b3/B3IndexMap.h:59
> > +    Vector<Value> m_vector;
> 
> Maybe it's worth giving this some inline storage?

Probably not.  IndexMap is usually used for BasicBlocks or Values.  Usually the total number of BasicBlocks is large, as is the total number of Values.  Inline storage is most useful when the total number of things is likely small.

> 
> > Source/JavaScriptCore/b3/B3IndexSet.h:54
> > +            return nullptr;
> 
> I would make this "return false"

Oops, fixed.

> 
> > Source/JavaScriptCore/b3/B3Opcode.h:71
> > +    ChillDiv, // doesn't trap ever, behaves like JS (x/y)|0.
> 
> Nice
> 
> > Source/JavaScriptCore/b3/B3Opcode.h:136
> > +    // It's legal to request that a stackmap argument is in some register and it's legal to request
> 
> Would it be possible to request it to be part of a certain set of
> constraints?

The code already does this, see B3Stackmap.h.

> Like: must be a callee save or on the stack?

You can absolutely request things to go to the stack. ValueRep::stackArgument() does this. Though, it only allows you to put things into the outgoing-argument area of the stack. I believe that's sensible because that's the only way we would use such a constraint, any any other kind of constraint would be damn hard to implement.

It doesn't make sense to add the callee save request, but it is possible to say that all non-callee-saves are clobbered. But, B3's stackmap has no early clobber. So, you might still get your arguments in volatile registers even if you claim to clobber non-callee-saves. Keeping things simple like this will allow for the register allocator to be lighter-weight, I think.

> 
> > Source/JavaScriptCore/b3/B3Opcode.h:166
> > +    // full branch fusion in the instruction selector. A true value jumps to the generator's slow
> 
> What is branch fusion?

If you say:

a = Equal(x, y)
Branch(a, target)

Then the naive codegen would be:

; Lowering of Equal:
cmp %x, %y
seteq %a
; Lowering of Branch:
test %a, %a
jnz target

Branch fusion is when you emit this instead:

cmp %x, %y
jeq target

In general "fusion" is another way of saying that the compiler matched a pattern that included more than one B3::Value and emitted just one instruction for all of those values - i.e. it fused them.

> 
> > Source/JavaScriptCore/b3/B3StackSlotKind.h:32
> > +    // A locked stack slot needs to be keep disjoint from all others because we intend to escape
> 
> "be keep" => "be"

Oops, I meant to write "be kept".

> 
> > Source/JavaScriptCore/b3/B3Stackmap.h:116
> > +    void* m_opaque { nullptr };
> 
> Maybe a better name for this is "m_identifier" or "m_uniqueIdentifier"?

Possibly, but I have another thing in mind that will be an identifier, for when we want to teach B3 how to do CSE on patchpoints. That would allow us to implement for example exotic math instructions (like sqrt) using Patchpoint. The "identifier" would be an int that "names" the operation that the Patchpoint performs. So, the rule will be that two Patchpoints are redundant if they are both pure, they have the same identifier, and they take the same operands.

That's why I used the name "opaque" for now. It's literally just a cookie that B3 passes through but doesn't interpret. We'd use it in the FTL to point to the IC or OSR data structure, or in the short term, to store what the FTL calls stackmapID.

> 
> > Source/JavaScriptCore/b3/B3Type.h:52
> > +    if (sizeof(void*) == 4)
> 
> Are we going to support 32-bit?

We don't have to support it. But in a super low-level compiler like this, it doesn't take a whole lot to get the support for free, so I'm just doing that.

> 
> > Source/JavaScriptCore/b3/B3ValueInlines.h:46
> > +template>typename T>
> 
> ">" => "<"

Oops.

[Filip Pizlo]

Created attachment 263934 [details]
it is written

Now I need to try to compile it...

[Lucas Forschler]

This thread deserves a blog entry.

[Filip Pizlo]

Created attachment 263937 [details]
added a feature flag, ENABLE(B3_JIT)

[Filip Pizlo]

Created attachment 263938 [details]
ready to be compiled

Lol there will be so many failures.

[Filip Pizlo]

Created attachment 263941 [details]
rebased

Starting to make it compile

[Benjamin Poulain]

(In reply to comment #37)
> > > Source/JavaScriptCore/jit/Reg.h:210
> > > +    static AllRegsIterable all() const { return AllRegsIterable(); }
> > 
> > The whole AllRegsIterable code seems unused, I don't see any call to all().
> 
> True. :-/  I added this because I saw too many loops like:
> 
>     for (Reg reg = Reg::first(); reg <= Reg::last(); reg = reg.next()) {
> 
> And I was *about* to write one in AirRegisterPriority.cpp. That's why I
> added this iterable.
> 
> What do you think of this patch fixing RegisterAtOffsetList to use this?

Sure. A few more lines won't stop you now :)

[Filip Pizlo]

Created attachment 263947 [details]
closer to compiling

[Benjamin Poulain]

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

> Source/JavaScriptCore/b3/B3AddressMatcher.patterns:35
> +AddShift1 = Add(Shl(index, $logScale), base)
> +AddShift2 = Add(base, Shl(index, $logScale))

I would prefer "#" instead of "$" for literals. It seems more common.

That would be adding complexity to the parser though...since # is also the start of comments.

> Source/JavaScriptCore/b3/generate_pattern_matcher.rb:131
> +        when /\A([ \t\r]+)/

"\A\s+"?

> Source/JavaScriptCore/b3/generate_pattern_matcher.rb:144
> +    # We can extend this as we add more keywords. Like, we might want "include" eventually.

Better already exclude "include" if you suspect you may use it in the future.

> Source/JavaScriptCore/b3/generate_pattern_matcher.rb:214
> +                parseError("Duplicate production")

It looks like this error would be associated with the wrong token. It would go with the first token of the next production, which would be misleading.

It may be better to just "raise" here directly with the faulty production name in the string.

> Source/JavaScriptCore/b3/generate_pattern_matcher.rb:295
> +    outp.puts "void run#{matcher.name}(Value* _value, Adaptor& adaptor)"

_value -> value?

> Source/JavaScriptCore/b3/generate_pattern_matcher.rb:300
> +    # Note that this does not emit properly indented code, because it's a recursive emitter. If you
> +    # want to see the code nicely formatted, open it in a good text editor and ask it to format it
> +    # for you.

Could be 
    # FIXME: indent the generated C++ code as expected.

[Filip Pizlo]

Created attachment 263962 [details]
it compiles!

It also sort of links, except that only works because nobody references anything in B3/Air yet.  Once I do that, it'll shake out the link errors.

[Filip Pizlo]

(In reply to comment #46)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263941&action=review
> 
> > Source/JavaScriptCore/b3/B3AddressMatcher.patterns:35
> > +AddShift1 = Add(Shl(index, $logScale), base)
> > +AddShift2 = Add(base, Shl(index, $logScale))
> 
> I would prefer "#" instead of "$" for literals. It seems more common.
> 
> That would be adding complexity to the parser though...since # is also the
> start of comments.

Hmm, really?  We already use $ for constants in bytecode, and $ is also used for constants in disassembly.

> 
> > Source/JavaScriptCore/b3/generate_pattern_matcher.rb:131
> > +        when /\A([ \t\r]+)/
> 
> "\A\s+"?

That would also catch newline, and would break line counting in the parser.

> 
> > Source/JavaScriptCore/b3/generate_pattern_matcher.rb:144
> > +    # We can extend this as we add more keywords. Like, we might want "include" eventually.
> 
> Better already exclude "include" if you suspect you may use it in the future.

Good call, added.

> 
> > Source/JavaScriptCore/b3/generate_pattern_matcher.rb:214
> > +                parseError("Duplicate production")
> 
> It looks like this error would be associated with the wrong token. It would
> go with the first token of the next production, which would be misleading.

Really?  This is just:

Foo = things...
Foo = more things...   <=== this is an error

> 
> It may be better to just "raise" here directly with the faulty production
> name in the string.
> 
> > Source/JavaScriptCore/b3/generate_pattern_matcher.rb:295
> > +    outp.puts "void run#{matcher.name}(Value* _value, Adaptor& adaptor)"
> 
> _value -> value?

Then I'd have to reserve the word "value".  The parser prohibits identifiers from starting with "_", so that we can prefix our own internal varaibles with "_".

> 
> > Source/JavaScriptCore/b3/generate_pattern_matcher.rb:300
> > +    # Note that this does not emit properly indented code, because it's a recursive emitter. If you
> > +    # want to see the code nicely formatted, open it in a good text editor and ask it to format it
> > +    # for you.
> 
> Could be 
>     # FIXME: indent the generated C++ code as expected.

I don't think we ever want to fix this, as that would make the code generator significantly less hackable without really gaining us anything.  It's so easy to autoindent the resulting text if you really want to read it!

[Benjamin Poulain]

(In reply to comment #48)
> (In reply to comment #46)
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=263941&action=review
> > 
> > > Source/JavaScriptCore/b3/B3AddressMatcher.patterns:35
> > > +AddShift1 = Add(Shl(index, $logScale), base)
> > > +AddShift2 = Add(base, Shl(index, $logScale))
> > 
> > I would prefer "#" instead of "$" for literals. It seems more common.
> > 
> > That would be adding complexity to the parser though...since # is also the
> > start of comments.
> 
> Hmm, really?  We already use $ for constants in bytecode, and $ is also used
> for constants in disassembly.

You probably look at x86 ASM more often, and I look at ARM ASM :)

> > > Source/JavaScriptCore/b3/generate_pattern_matcher.rb:214
> > > +                parseError("Duplicate production")
> > 
> > It looks like this error would be associated with the wrong token. It would
> > go with the first token of the next production, which would be misleading.
> 
> Really?  This is just:
> 
> Foo = things...
> Foo = more things...   <=== this is an error

Ok. I thought "token" would be the token *after* the production.

[Filip Pizlo]

(In reply to comment #49)
> (In reply to comment #48)
> > (In reply to comment #46)
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=263941&action=review
> > > 
> > > > Source/JavaScriptCore/b3/B3AddressMatcher.patterns:35
> > > > +AddShift1 = Add(Shl(index, $logScale), base)
> > > > +AddShift2 = Add(base, Shl(index, $logScale))
> > > 
> > > I would prefer "#" instead of "$" for literals. It seems more common.
> > > 
> > > That would be adding complexity to the parser though...since # is also the
> > > start of comments.
> > 
> > Hmm, really?  We already use $ for constants in bytecode, and $ is also used
> > for constants in disassembly.
> 
> You probably look at x86 ASM more often, and I look at ARM ASM :)

Lol, that's a good point.

The more I think about it, I might remove the $ notation.  If we want to match a constant (like, Equal(x, 0) will be a thing, since it means "not"), then maybe we can write the constant without any prefix.  The purpose of $ is that you can match *any* constant.  But in those cases, it's so easy to just say "if (thingy->hasInt32())" or whatever inside the C++ code.

I'll keep this for now, since I'm so close to this being landable.  But I've filed a bug about the possible uselessness of $: https://bugs.webkit.org/show_bug.cgi?id=150527

> 
> > > > Source/JavaScriptCore/b3/generate_pattern_matcher.rb:214
> > > > +                parseError("Duplicate production")
> > > 
> > > It looks like this error would be associated with the wrong token. It would
> > > go with the first token of the next production, which would be misleading.
> > 
> > Really?  This is just:
> > 
> > Foo = things...
> > Foo = more things...   <=== this is an error
> 
> Ok. I thought "token" would be the token *after* the production.

[Filip Pizlo]

Created attachment 263974 [details]
it links!

[Benjamin Poulain]

Comment on attachment 263974 [details]
it links!

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

Some more small comments.

> Source/JavaScriptCore/assembler/MacroAssembler.h:1610
> +namespace JSC {

Oops :)

> Source/JavaScriptCore/b3/B3ArgumentRegValue.h:53
> +    {

ASSERT(reg.isSet());

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:302
> +                // This production isn't strictly necessary since we expect
> +                // Load(Add(@x, @const1), offset = const2) to have already been strength-reduced
> +                // to Load(@x, offset = const1 + const2).
> +                selectedAddress = Arg::addr(lower.tmp(left), right->asInt32());

If strength reduction already takes care of that, IMHO you should just leave this out.

> Source/JavaScriptCore/b3/B3Opcode.h:54
> +    // The magical argument register. This is viewed as executing at the top of the program
> +    // regardless of where in control flow you put it, and the compiler takes care to ensure that we

"This is viewed as executing at the top of the program regardless of where in control flow you put it".

Wouldn't it be easier to be super strict about those: they must appear as the first statements inside the first BasicBlock? That way you can use regular analysis on them.

> Source/JavaScriptCore/b3/B3Type.h:53
> +inline Type pointer()

pointer() -> pointerType()?

The code using "pointer()" looks a bit weird.

[Filip Pizlo]

It just did its first compile!

The test:

void test42()
{
    Procedure proc;
    BasicBlock* root = proc.addBlock();
    Value* const42 = root->appendNew<Const32Value>(proc, Origin(), 42);
    root->appendNew<ControlValue>(proc, Return, Origin(), const42);

    CHECK(compileAndRun<int>(proc) == 42);
}

The output with full dumping enabled:

[pizlo@shakezilla OpenSource] JSC_dumpGraphAtEachPhase=true JSC_dumpDisassembly=true JSC_validateGraphAtEachPhase=true DYLD_FRAMEWORK_PATH=WebKitBuild/Release/ WebKitBuild/Release/testb3 
test42():
Initial B3:
BB#0: ; frequency = nan
    Int32 @0 = Const32(42)
    Void @1 = Return(@0)
B3 before generation:
BB#0: ; frequency = nan
    Int32 @0 = Const32(42)
    Void @1 = Return(@0)
B3 before lowerToAir:
BB#0: ; frequency = nan
    Int32 @0 = Const32(42)
    Void @1 = Return(@0)
Initial air:
BB#0: ; frequency = nan
    Move $42, %tmp0, @0
    Move $42, %rax, @1
    Ret @1
Frame size: 0
Call arg area size: 0
Callee saves: 
Air before spillEverything:
BB#0: ; frequency = nan
    Move $42, %tmp0, @0
    Move $42, %rax, @1
    Ret @1
Frame size: 0
Call arg area size: 0
Callee saves: 
Air before handleCalleeSaves:
BB#0: ; frequency = nan
    Move $42, %rax, @0
    Move %rax, (stack0), @0
    Move $42, %rax, @1
    Ret @1
Stack slots:
    byteSize = 8, offsetFromFP = 0, kind = Anonymous
Frame size: 0
Call arg area size: 0
Callee saves: 
Air before allocateStack:
BB#0: ; frequency = nan
    Move $42, %rax, @0
    Move %rax, (stack0), @0
    Move $42, %rax, @1
    Ret @1
Stack slots:
    byteSize = 8, offsetFromFP = 0, kind = Anonymous
    byteSize = 0, offsetFromFP = 0, kind = Locked
Frame size: 0
Call arg area size: 0
Callee saves: 
Air before generation:
BB#0: ; frequency = nan
    Move $42, %rax, @0
    Move %rax, -8(%rbp), @0
    Move $42, %rax, @1
    Ret @1
Stack slots:
    byteSize = 8, offsetFromFP = -8, kind = Anonymous
    byteSize = 0, offsetFromFP = 0, kind = Locked
Frame size: 16
Call arg area size: 0
Callee saves: 
Generated JIT code for testb3:
    Code at [0x4aba39a01000, 0x4aba39a01020):
      0x4aba39a01000: push %rbp
      0x4aba39a01001: mov %rsp, %rbp
      0x4aba39a01004: add $0xfffffffffffffff0, %rsp
      0x4aba39a01008: mov $0x2a, %eax
      0x4aba39a0100d: mov %rax, -0x8(%rbp)
      0x4aba39a01011: mov $0x2a, %eax
      0x4aba39a01016: mov %rbp, %rsp
      0x4aba39a01019: pop %rbp
      0x4aba39a0101a: ret 
    OK!

[Filip Pizlo]

(In reply to comment #52)
> Comment on attachment 263974 [details]
> it links!
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263974&action=review
> 
> Some more small comments.
> 
> > Source/JavaScriptCore/assembler/MacroAssembler.h:1610
> > +namespace JSC {
> 
> Oops :)
> 
> > Source/JavaScriptCore/b3/B3ArgumentRegValue.h:53
> > +    {
> 
> ASSERT(reg.isSet());

Fixed!

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:302
> > +                // This production isn't strictly necessary since we expect
> > +                // Load(Add(@x, @const1), offset = const2) to have already been strength-reduced
> > +                // to Load(@x, offset = const1 + const2).
> > +                selectedAddress = Arg::addr(lower.tmp(left), right->asInt32());
> 
> If strength reduction already takes care of that, IMHO you should just leave
> this out.

We don't have strength reduction yet. :-)  I'm still torn about whether I want this to be a rule.  If it really was the case that strength reduction was the *very last* phase before LowerToAir, then we could argue that we should never put any strength reductions into LowerToAir.  But I'm not sure we should mandate that strength reduction is the last phase, because we will probably end up adding "late" fixup phases (DFG has a lot of these as does LLVM).

> 
> > Source/JavaScriptCore/b3/B3Opcode.h:54
> > +    // The magical argument register. This is viewed as executing at the top of the program
> > +    // regardless of where in control flow you put it, and the compiler takes care to ensure that we
> 
> "This is viewed as executing at the top of the program regardless of where
> in control flow you put it".
> 
> Wouldn't it be easier to be super strict about those: they must appear as
> the first statements inside the first BasicBlock? That way you can use
> regular analysis on them.

Actually, you can already do regular analysis on these. :-)  They behave like pure values.

> 
> > Source/JavaScriptCore/b3/B3Type.h:53
> > +inline Type pointer()
> 
> pointer() -> pointerType()?
> 
> The code using "pointer()" looks a bit weird.

Sure, I'll change it.

[Filip Pizlo]

Created attachment 263976 [details]
the patch

[WebKit Commit Bot]

Attachment 263976 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:104:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:118:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3StackSlotValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3Procedure.cpp:94:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:87:  Extra space between return and false  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/b3/B3PatchpointValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:71:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:72:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:123:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:124:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBasicBlock.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:113:  The parameter name "switchCase" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:121:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirStackSlot.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:67:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:110:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.h:49:  The parameter name "opcode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3BasicBlock.h:26:  #ifndef header guard has wrong style, please use: B3BasicBlock_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:122:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 46 in 133 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 263982 [details]
the patch

Maybe it'll even build this time.

[WebKit Commit Bot]

Attachment 263982 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:104:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:118:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3StackSlotValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3Procedure.cpp:94:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:87:  Extra space between return and false  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/b3/B3PatchpointValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBasicBlock.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:113:  The parameter name "switchCase" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:121:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirStackSlot.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:67:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:110:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.h:49:  The parameter name "opcode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:129:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3BasicBlock.h:26:  #ifndef header guard has wrong style, please use: B3BasicBlock_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:122:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 46 in 134 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

Comment on attachment 263976 [details]
the patch

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

Be patient, it's a big patch :)

> Source/JavaScriptCore/b3/B3BasicBlock.h:46
> +    typedef Vector<FrequentedBlock, 2> SuccessorList; // This matches ControlValue::SuccessorList

Instead of a comment, you could declare a BasicBlockSuccessorList type in B3Type and typedef from it in BasicBlock and ControlValue.

> Source/JavaScriptCore/b3/B3BasicBlock.h:75
> +    FrequentedBlock successor(unsigned index) const;

Return
    const FrequentedBlock&
instead?

> Source/JavaScriptCore/b3/B3BasicBlock.h:93
> +    bool addPredecessor(BasicBlock*);
> +    bool removePredecessor(BasicBlock*);
> +    bool replacePredecessor(BasicBlock* from, BasicBlock* to);

It would be nice if those function could ASSERT that the new relation make sense. It could check that the new predecessor's last Value* has this block as a possible target.

removePredecessor could check that we are no longer a target.

> Source/JavaScriptCore/b3/B3BasicBlockUtils.h:56
> +            predecessors[i--] = predecessors.last();
> +            predecessors.removeLast();

This could be 
    predecessors[i--] = predecessors.takeLast();

> Source/JavaScriptCore/b3/B3FrequentedBlock.h:37
> +typedef GenericFrequentedBlock<BasicBlock> FrequentedBlock;

Frequented sounds weird to me for "something-with-a-frequency". English is not my forte though :)

Maybe QuantifiedBlock or WeightedBlock?

> Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:49
> +    bool operator==(const GenericFrequentedBlock& other) const

Do you need to declare this and the operator!=?
Aren't they doing the same as the generated ones?

> Source/JavaScriptCore/b3/B3SuccessorCollection.h:35
> +template<typename BasicBlock, typename SuccessorList>

Do you need the type BasicBlock?

You may be able to just use "auto" to get the right type out of SuccessorList.

> Source/JavaScriptCore/b3/B3SuccessorCollection.h:36
> +class SuccessorCollection {

Since the "frequented" successor list is named SuccessorList, 
    SuccessorCollection->SuccessorBlockList?

> Source/JavaScriptCore/b3/B3SuccessorCollection.h:58
> +            : m_collection(const_cast<SuccessorCollection*>(&collection)) // const is stupid anyway.

Why do you take the collection as const anyway?

Your begin() and end() are not const.

[Filip Pizlo]

(In reply to comment #59)
> Comment on attachment 263976 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263976&action=review
> 
> Be patient, it's a big patch :)

Oh it'll take a while before I even get it to build everywhere!

> 
> > Source/JavaScriptCore/b3/B3BasicBlock.h:46
> > +    typedef Vector<FrequentedBlock, 2> SuccessorList; // This matches ControlValue::SuccessorList
> 
> Instead of a comment, you could declare a BasicBlockSuccessorList type in
> B3Type and typedef from it in BasicBlock and ControlValue.

That would require having BasicBlock.h include ControlValue.h, or vice versa.  I did this matching declaration of SuccessorList instead, so that we don't have an include dependency.  I figure that as the code grows, we'll discover some good reason for one of these headers to include the other, and it's better to leave it flexible for now.

> 
> > Source/JavaScriptCore/b3/B3BasicBlock.h:75
> > +    FrequentedBlock successor(unsigned index) const;
> 
> Return
>     const FrequentedBlock&
> instead?

I changed it.  Though, it probably doesn't make a huge difference since the code gets inlined anyway.  If we use FrequentedBlock instead of const FrequentedBlock&, then after all of the inlining, you'll first have something like:

FrequentedBlock x = block->last()->as<ControlValue>()->m_successors[index];

But then after SROA, you get:

const FrequentedBlock& xRef = block->last()->as<ControlValue>()->m_successors[index];
BasicBlock* xBlock = xRef.m_block;
FrequencyClass xFreq = xRef.m_frequency;

If you only use the block but not the frequency (or vice versa) then you'll only do that load.

In other words, it really shouldn't matter if the code is inlineable.

> 
> > Source/JavaScriptCore/b3/B3BasicBlock.h:93
> > +    bool addPredecessor(BasicBlock*);
> > +    bool removePredecessor(BasicBlock*);
> > +    bool replacePredecessor(BasicBlock* from, BasicBlock* to);
> 
> It would be nice if those function could ASSERT that the new relation make
> sense. It could check that the new predecessor's last Value* has this block
> as a possible target.
> 
> removePredecessor could check that we are no longer a target.

If we did that, then you'd have to always edit the successors before you edit the predecessors.  That seems arbitrary.  Why not the other way around? ;-)

This could easily get very annoying - you might have a transformation for which it's most convenient to edit the successors/predecessors in a particular order, and in that case we wouldn't want assertions to prevent us from using the most natural approach.

We can easily add a validation rule for predecessors.  If you make a mistake, you'll know exactly which phase caused it.  So, I think that BasicBlock should give you maximum flexibility to temporarily put the IR in an invalid state.  This is how the IR works in all other regards.

> 
> > Source/JavaScriptCore/b3/B3BasicBlockUtils.h:56
> > +            predecessors[i--] = predecessors.last();
> > +            predecessors.removeLast();
> 
> This could be 
>     predecessors[i--] = predecessors.takeLast();

No, it can't.  That would crash if predecessors had one element left and we were removing it.  takeLast() would make predecessors have zero elements, and then "predecessors[i--] = ..." would crash because i would be zero, the size would be zero, and if we're sufficiently unlucky we would have already deleted the backing store.  Even if we didn't delete the backing store, you'd get one of those asserts with security implications. ;-)

> 
> > Source/JavaScriptCore/b3/B3FrequentedBlock.h:37
> > +typedef GenericFrequentedBlock<BasicBlock> FrequentedBlock;
> 
> Frequented sounds weird to me for "something-with-a-frequency". English is
> not my forte though :)
> 
> Maybe QuantifiedBlock or WeightedBlock?

I was going for a slightly weird name.  I wanted to call it BlockEdgeWithFrequency.  In the DFG, I called it BranchTarget, which ended up being a suboptimal name because I can never remember what exactly it means.

My hope was that FrequentedBlock sounds weird but is memorable enough that you'll instinctively know what it means.

QuantifiedBlock doesn't quite tell me that we're talking about the frequency.

WeightedBlock kind of tells me what this is about, but to me it sounds more like the block itself has a weight.  That's not true.  It's the edge to the block that has a "weight" (or frequency, in the current jargon).

"FrequentedBlock" also has the benefit that by virtue of being a goofy phrase, it alerts you to the fact that it's not the block that has the frequency.  The frequency tells us how often this predecessor jumps to that block.

> 
> > Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:49
> > +    bool operator==(const GenericFrequentedBlock& other) const
> 
> Do you need to declare this and the operator!=?
> Aren't they doing the same as the generated ones?

I didn't know that the compiler synthesizes these!

> 
> > Source/JavaScriptCore/b3/B3SuccessorCollection.h:35
> > +template<typename BasicBlock, typename SuccessorList>
> 
> Do you need the type BasicBlock?
> 
> You may be able to just use "auto" to get the right type out of
> SuccessorList.

Well, I need to have a type declaration for the return values of at() and operator[]().  I don't think I can put auto as the return value.

> 
> > Source/JavaScriptCore/b3/B3SuccessorCollection.h:36
> > +class SuccessorCollection {
> 
> Since the "frequented" successor list is named SuccessorList, 
>     SuccessorCollection->SuccessorBlockList?

Well, so far when we say "BlahList", it means that it's a Vector, oddly enough.  At the very list, it means that it's a list type that actually supports all of the list operations you'd expect to see.  SuccessorCollection doesn't do that.  It allows for forward iteration and random access, but no appending or removing.  That's why I went for "Collection" instead of "List" - it doesn't really do all of the things you'd expect a List to do.

> 
> > Source/JavaScriptCore/b3/B3SuccessorCollection.h:58
> > +            : m_collection(const_cast<SuccessorCollection*>(&collection)) // const is stupid anyway.
> 
> Why do you take the collection as const anyway?
> 
> Your begin() and end() are not const.

I don't remember.  I'll fiddle with this again.  So far, so good - removing the const and the cast appears to work.

[Filip Pizlo]

Created attachment 263993 [details]
the patch

Lots of fixes.

[Filip Pizlo]

Created attachment 263994 [details]
the patch

Rebased

[WebKit Commit Bot]

Attachment 263994 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:104:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:118:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3StackSlotValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3Procedure.cpp:94:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:87:  Extra space between return and false  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/b3/B3PatchpointValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBasicBlock.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:113:  The parameter name "switchCase" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:121:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirStackSlot.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:68:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:110:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.h:49:  The parameter name "opcode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:129:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3BasicBlock.h:26:  #ifndef header guard has wrong style, please use: B3BasicBlock_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 46 in 135 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 263996 [details]
the patch

More fixes.

[WebKit Commit Bot]

Attachment 263996 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:104:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:118:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3StackSlotValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3Procedure.cpp:94:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:87:  Extra space between return and false  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/b3/B3PatchpointValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBasicBlock.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:113:  The parameter name "switchCase" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:121:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirStackSlot.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:68:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:110:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.h:49:  The parameter name "opcode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:129:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3BasicBlock.h:26:  #ifndef header guard has wrong style, please use: B3BasicBlock_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 46 in 135 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 263997 [details]
the patch

More fixes for !ENABLE(B3_JIT).

[WebKit Commit Bot]

Attachment 263997 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:104:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:118:  The parameter name "functor" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3StackSlotValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3Procedure.cpp:94:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirCCallSpecial.cpp:87:  Extra space between return and false  [whitespace/declaration] [3]
ERROR: Source/JavaScriptCore/b3/B3PatchpointValue.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:118:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirBasicBlock.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:113:  The parameter name "switchCase" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3SwitchValue.h:121:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirStackSlot.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:70:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:110:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.h:49:  The parameter name "opcode" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:129:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3Opcode.h:38:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3BasicBlock.h:26:  #ifndef header guard has wrong style, please use: B3BasicBlock_h  [build/header_guard] [5]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 46 in 135 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 264000 [details]
the patch

Fixed a bunch of style issues.

But >2/3 of the style issues that were reported are invalid.  The style checker doesn't know our C++11 style yet.

[WebKit Commit Bot]

Attachment 264000 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:70:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:129:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 29 in 135 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 264047 [details]
the patch

Did some minor simplifications, filed more bugs, linked the bugs with FIXMEs and comments, and rebased.

[WebKit Commit Bot]

Attachment 264047 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:70:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:129:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 29 in 135 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 264051 [details]
the patch

Made the dumps easier to read.

[WebKit Commit Bot]

Attachment 264051 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:98:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:70:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:129:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 29 in 135 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 264097 [details]
the patch

Lots more test coverage.

[WebKit Commit Bot]

Attachment 264097 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:98:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:85:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:140:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:123:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 30 in 137 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

This is so cool.  Notice the epic ArgumentReg opcode in action.


testStore(44):
B3 after initial, before lowerToAir:
BB#0: ; frequency = nan
    Int64 @0 = ArgumentReg(%rdi)
    Int32 @1 = Trunc(@0)
    Int64 @2 = Const64(140734534183196)
    Void @3 = Store(@1, @2)
    Int32 @4 = Const32(0)
    Void @5 = Return(@4)
Air after initial, before spillEverything:
BB#0: ; frequency = nan
    Move %rdi, %tmp3, @0
    Move %tmp3, %tmp1, @1
    Move $0x7fff4feae91c, %tmp2, @2
    Move32 %tmp1, (%tmp2), @3
    Move $0, %tmp0, @4
    Move $0, %rax, @5
    Ret @5
Air after spillEverything, before handleCalleeSaves:
BB#0: ; frequency = nan
    Move %rdi, %rax, @0
    Move %rax, (stack0), @0
    Move (stack0), %rcx, @1
    Move %rcx, %rax, @1
    Move %rax, (stack2), @1
    Move $0x7fff4feae91c, %rax, @2
    Move %rax, (stack1), @2
    Move (stack2), %rcx, @3
    Move (stack1), %rdx, @3
    Move32 %rcx, (%rdx), @3
    Move $0, %rax, @4
    Move %rax, (stack3), @4
    Move $0, %rax, @5
    Ret @5
Stack slots:
    stack0: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack1: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack2: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack3: byteSize = 8, offsetFromFP = 0, kind = Anonymous
Air after handleCalleeSaves, before allocateStack:
BB#0: ; frequency = nan
    Move %rdi, %rax, @0
    Move %rax, (stack0), @0
    Move (stack0), %rcx, @1
    Move %rcx, %rax, @1
    Move %rax, (stack2), @1
    Move $0x7fff4feae91c, %rax, @2
    Move %rax, (stack1), @2
    Move (stack2), %rcx, @3
    Move (stack1), %rdx, @3
    Move32 %rcx, (%rdx), @3
    Move $0, %rax, @4
    Move %rax, (stack3), @4
    Move $0, %rax, @5
    Ret @5
Stack slots:
    stack0: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack1: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack2: byteSize = 8, offsetFromFP = 0, kind = Anonymous
    stack3: byteSize = 8, offsetFromFP = 0, kind = Anonymous
Air after allocateStack, before generation:
BB#0: ; frequency = nan
    Move %rdi, %rax, @0
    Move %rax, -8(%rbp), @0
    Move -8(%rbp), %rcx, @1
    Move %rcx, %rax, @1
    Move %rax, -16(%rbp), @1
    Move $0x7fff4feae91c, %rax, @2
    Move %rax, -8(%rbp), @2
    Move -16(%rbp), %rcx, @3
    Move -8(%rbp), %rdx, @3
    Move32 %rcx, (%rdx), @3
    Move $0, %rax, @4
    Move %rax, -8(%rbp), @4
    Move $0, %rax, @5
    Ret @5
Stack slots:
    stack0: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack1: byteSize = 8, offsetFromFP = -8, kind = Anonymous
    stack2: byteSize = 8, offsetFromFP = -16, kind = Anonymous
    stack3: byteSize = 8, offsetFromFP = -8, kind = Anonymous
Frame size: 16
Generated JIT code for testb3:
    Code at [0x564c6d401000, 0x564c6d401040):
      0x564c6d401000: push %rbp
      0x564c6d401001: mov %rsp, %rbp
      0x564c6d401004: add $0xfffffffffffffff0, %rsp
      0x564c6d401008: mov %rdi, %rax
      0x564c6d40100b: mov %rax, -0x8(%rbp)
      0x564c6d40100f: mov -0x8(%rbp), %rcx
      0x564c6d401013: mov %rcx, %rax
      0x564c6d401016: mov %rax, -0x10(%rbp)
      0x564c6d40101a: mov $0x7fff4feae91c, %rax
      0x564c6d401024: mov %rax, -0x8(%rbp)
      0x564c6d401028: mov -0x10(%rbp), %rcx
      0x564c6d40102c: mov -0x8(%rbp), %rdx
      0x564c6d401030: mov %ecx, (%rdx)
      0x564c6d401032: xor %eax, %eax
      0x564c6d401034: mov %rax, -0x8(%rbp)
      0x564c6d401038: xor %eax, %eax
      0x564c6d40103a: mov %rbp, %rsp
      0x564c6d40103d: pop %rbp
      0x564c6d40103e: ret 
    OK!

[Benjamin Poulain]

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

Some more feedback. Some comments are probably useless, I don't have the context for everything yet.

> Source/JavaScriptCore/b3/B3ArgumentRegValue.h:36
> +class ArgumentRegValue : public Value {

Missing JS_EXPORT_PRIVATE?

> Source/JavaScriptCore/b3/B3CheckSpecial.cpp:63
> +{

ASSERT opcode is one of the 4 valid ones?

> Source/JavaScriptCore/b3/B3CheckSpecial.cpp:75
> +    hiddenBranch.args.resize(m_numCheckArgs);
> +    for (unsigned i = m_numCheckArgs; i--;)
> +        hiddenBranch.args[i] = inst.args[i + 1];

Maybe
    hiddenBranch.args.reserveInitialCapacity(m_numCheckArgs);
    for (unsigned i = 0; i < m_numCheckArgs; ++i)
        hiddenBranch.args.appendUnchecked(inst.args[i + 1]);
To avoid calling the Arg() constructor for every cell of hiddenBranch.args.resize().

> Source/JavaScriptCore/b3/B3CheckSpecial.h:34
> +namespace JSC { namespace B3 {

Should this be in the Air namespace?

> Source/JavaScriptCore/b3/B3CheckSpecial.h:43
> +// Note that for CheckAdd, CheckSub, and CheckMul we expect that the B3 arguments are the reverse
> +// of the Air arguments (Add(a, b) => Add32 b, a). Except:

This is a weird convention.
Aren't you talking specifically about x86 Integer lowering?

> Source/JavaScriptCore/b3/B3CheckSpecial.h:53
> +    Air::Inst hiddenBranch(const Air::Inst&) const;

Looks like it could be private.

> Source/JavaScriptCore/b3/B3CheckValue.h:36
> +class CheckValue : public Value {

Aren't you missing the JS_EXPORT_PRIVATE?

> Source/JavaScriptCore/b3/B3ComparisonMatcher.patterns:24
> +matcher ComparisonMatcher

I don't see this Matcher being used anywhere. Not sure what's your plan for it.

> Source/JavaScriptCore/b3/B3Const64Value.h:35
> +class Const64Value : public Value {

JS_EXPORT_PRIVATE?

> Source/JavaScriptCore/b3/B3Stackmap.h:43
> +class Stackmap {
> +public:

If I am not mistaken, this is never copied at the moment. That's good since it contains a Vector.

It may be worth adding WTF_MAKE_NONCOPYABLE() to safe-proof future uses.

> Source/JavaScriptCore/b3/B3Stackmap.h:61
> +    Stackmap()

Could be out of line.

> Source/JavaScriptCore/b3/B3Stackmap.h:64
> +

Missing an out of line destructor on this one.

> Source/JavaScriptCore/b3/B3Stackmap.h:81
> +    // Set some opaque data to identify this stackmap. This is useful because a Stackmap-using Value
> +    // can be duplicated, and each duplicate may end up having different register allocation. This
> +    // allows all of those duplicates to be able to refer back to whatever high-level data structures
> +    // we were using to create the value.
> +    void setOpaque(void* parameter)
> +    {
> +        m_opaque = parameter;
> +    }
> +    void* opaque() const { return m_opaque; }
> +
> +    // Convenience helpers when using indices instead of pointers.
> +    void setOpaqueIndex(size_t index)
> +    {
> +        setOpaque(bitwise_cast<void*>(index));
> +    }
> +    size_t opaqueIndex() const { return bitwise_cast<size_t>(opaque()); }
> +

The opaque data looks unused.
Let's remove it. It's easy to add back when it will be needed.

> Source/JavaScriptCore/b3/B3Stackmap.h:90
> +        if (index + 1 < m_reps.size())

Wrong sign?
    index + 1 > m_reps.size()
or
    index >= m_reps.size()

> Source/JavaScriptCore/b3/B3Stackmap.h:122
> +    RefPtr<Generator> m_generator;

I assume that's the Special's generator?
I find it a bit weird that it is on the StackMap. It may make more sense as I read more of the context.

> Source/JavaScriptCore/b3/B3ValueRep.h:154
> +    int64_t m_value;

I think this would look cleaner as an Union of Reg and intptr_t.
union {
    Reg register,
    intptr_t offsetFromFP,
    intptr_t offsetFromSP,
} m_value;

> Source/JavaScriptCore/b3/air/AirSpecial.h:96
> +    unsigned m_index;

I don't see this being used anywhere.

[Filip Pizlo]

(In reply to comment #77)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264051&action=review
> 
> Some more feedback. Some comments are probably useless, I don't have the
> context for everything yet.
> 
> > Source/JavaScriptCore/b3/B3ArgumentRegValue.h:36
> > +class ArgumentRegValue : public Value {
> 
> Missing JS_EXPORT_PRIVATE?

Yeah, I'll make sure I add it. I may have added it in the last version. 

> 
> > Source/JavaScriptCore/b3/B3CheckSpecial.cpp:63
> > +{
> 
> ASSERT opcode is one of the 4 valid ones?

Sure!

> 
> > Source/JavaScriptCore/b3/B3CheckSpecial.cpp:75
> > +    hiddenBranch.args.resize(m_numCheckArgs);
> > +    for (unsigned i = m_numCheckArgs; i--;)
> > +        hiddenBranch.args[i] = inst.args[i + 1];
> 
> Maybe
>     hiddenBranch.args.reserveInitialCapacity(m_numCheckArgs);
>     for (unsigned i = 0; i < m_numCheckArgs; ++i)
>         hiddenBranch.args.appendUnchecked(inst.args[i + 1]);
> To avoid calling the Arg() constructor for every cell of
> hiddenBranch.args.resize().

That's interesting. I can do that. 

> 
> > Source/JavaScriptCore/b3/B3CheckSpecial.h:34
> > +namespace JSC { namespace B3 {
> 
> Should this be in the Air namespace?

Ugh this code is dead for now so it's hard to see the context. It was important for me to write it because I needed to see that it made sense, and in fact I changed how Air worked as a result of this exercise. I think it's better to land this dead code though, so that our expectations from the compiler for the purpose of patchpoints are documented in a compiler-enforced manner. 

That said, the way this will work is that this is sort of private to B3LowerToAir.cpp. I'm usually erring on the side of using the B3 namespace for code that knows about both Air and B3. 

> 
> > Source/JavaScriptCore/b3/B3CheckSpecial.h:43
> > +// Note that for CheckAdd, CheckSub, and CheckMul we expect that the B3 arguments are the reverse
> > +// of the Air arguments (Add(a, b) => Add32 b, a). Except:
> 
> This is a weird convention.
> Aren't you talking specifically about x86 Integer lowering?

This is the convention that our MacroAssembler uses, and Air just follows that. 

> 
> > Source/JavaScriptCore/b3/B3CheckSpecial.h:53
> > +    Air::Inst hiddenBranch(const Air::Inst&) const;
> 
> Looks like it could be private.

Sure!

> 
> > Source/JavaScriptCore/b3/B3CheckValue.h:36
> > +class CheckValue : public Value {
> 
> Aren't you missing the JS_EXPORT_PRIVATE?

Yes. 

> 
> > Source/JavaScriptCore/b3/B3ComparisonMatcher.patterns:24
> > +matcher ComparisonMatcher
> 
> I don't see this Matcher being used anywhere. Not sure what's your plan for
> it.

I will remove it. 

> 
> > Source/JavaScriptCore/b3/B3Const64Value.h:35
> > +class Const64Value : public Value {
> 
> JS_EXPORT_PRIVATE?

Yes. 

> 
> > Source/JavaScriptCore/b3/B3Stackmap.h:43
> > +class Stackmap {
> > +public:
> 
> If I am not mistaken, this is never copied at the moment. That's good since
> it contains a Vector.
> 
> It may be worth adding WTF_MAKE_NONCOPYABLE() to safe-proof future uses.

It's meant to be copied. :-)

One of the principles that a good compiler IR should follow is that it should make it possible to easily duplicate code. This is necessary for things like loop peeling, loop unrolling, tail duplication, jump threading, and path specialization. All of those also happen to be classic optimizations for high level languages. 

All B3::Value classes are already designed to make copying easy. The code for cloning a Value isn't there yet, and we may never need those optimizations. But the duplication capability is something you sort of need to bake into the IR from the start, for example by making sure that any data setructure owned by a Value is copyable. 

> 
> > Source/JavaScriptCore/b3/B3Stackmap.h:61
> > +    Stackmap()
> 
> Could be out of line.

Sure!

> 
> > Source/JavaScriptCore/b3/B3Stackmap.h:64
> > +
> 
> Missing an out of line destructor on this one.

Sure. 

> 
> > Source/JavaScriptCore/b3/B3Stackmap.h:81
> > +    // Set some opaque data to identify this stackmap. This is useful because a Stackmap-using Value
> > +    // can be duplicated, and each duplicate may end up having different register allocation. This
> > +    // allows all of those duplicates to be able to refer back to whatever high-level data structures
> > +    // we were using to create the value.
> > +    void setOpaque(void* parameter)
> > +    {
> > +        m_opaque = parameter;
> > +    }
> > +    void* opaque() const { return m_opaque; }
> > +
> > +    // Convenience helpers when using indices instead of pointers.
> > +    void setOpaqueIndex(size_t index)
> > +    {
> > +        setOpaque(bitwise_cast<void*>(index));
> > +    }
> > +    size_t opaqueIndex() const { return bitwise_cast<size_t>(opaque()); }
> > +
> 
> The opaque data looks unused.
> Let's remove it. It's easy to add back when it will be needed.

Actually it's completely redundant since the client could encapsulate it using the m_generator b

> 
> > Source/JavaScriptCore/b3/B3Stackmap.h:90
> > +        if (index + 1 < m_reps.size())
> 
> Wrong sign?
>     index + 1 > m_reps.size()
> or
>     index >= m_reps.size()

Oops. 

> 
> > Source/JavaScriptCore/b3/B3Stackmap.h:122
> > +    RefPtr<Generator> m_generator;
> 
> I assume that's the Special's generator?
> I find it a bit weird that it is on the StackMap. It may make more sense as
> I read more of the context.

Where else would it be?  The idea is that the client that creates a patchpoint or CheckValue needs to supply a generator. 

> 
> > Source/JavaScriptCore/b3/B3ValueRep.h:154
> > +    int64_t m_value;
> 
> I think this would look cleaner as an Union of Reg and intptr_t.
> union {
>     Reg register,
>     intptr_t offsetFromFP,
>     intptr_t offsetFromSP,
> } m_value;

But I can't have Reg in the union!

> 
> > Source/JavaScriptCore/b3/air/AirSpecial.h:96
> > +    unsigned m_index;
> 
> I don't see this being used anywhere.

Dumping.

[Filip Pizlo]

> > 
> > > Source/JavaScriptCore/b3/B3CheckSpecial.cpp:63
> > > +{
> > 
> > ASSERT opcode is one of the 4 valid ones?
> 
> Sure!

Actually, no.  This could be any branching opcode.  The B3::Opcode is one of the 4 valid ones, but the Air::Opcode could be any branch.  I guess I can add an ASSERT that it's a branch.

[Filip Pizlo]

Created attachment 264121 [details]
the patch

More tests, more little fixes.

[WebKit Commit Bot]

Attachment 264121 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:98:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 32 in 136 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 264122 [details]
the patch

With build fixes.

[WebKit Commit Bot]

Attachment 264122 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:98:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 32 in 136 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

(In reply to comment #78)
> > > Source/JavaScriptCore/b3/B3Stackmap.h:122
> > > +    RefPtr<Generator> m_generator;
> > 
> > I assume that's the Special's generator?
> > I find it a bit weird that it is on the StackMap. It may make more sense as
> > I read more of the context.
> 
> Where else would it be?  The idea is that the client that creates a
> patchpoint or CheckValue needs to supply a generator. 

I was expecting it on the value.
I'll need some more reading to have a informed opinion. My understanding of the Specials is still very high level.

> > > Source/JavaScriptCore/b3/B3ValueRep.h:154
> > > +    int64_t m_value;
> > 
> > I think this would look cleaner as an Union of Reg and intptr_t.
> > union {
> >     Reg register,
> >     intptr_t offsetFromFP,
> >     intptr_t offsetFromSP,
> > } m_value;
> 
> But I can't have Reg in the union!

You can with C++11. It is used a bit in WebCore. You just need to be explicit about construction/destruction.

[Filip Pizlo]

(In reply to comment #84)
> (In reply to comment #78)
> > > > Source/JavaScriptCore/b3/B3Stackmap.h:122
> > > > +    RefPtr<Generator> m_generator;
> > > 
> > > I assume that's the Special's generator?
> > > I find it a bit weird that it is on the StackMap. It may make more sense as
> > > I read more of the context.
> > 
> > Where else would it be?  The idea is that the client that creates a
> > patchpoint or CheckValue needs to supply a generator. 
> 
> I was expecting it on the value.
> I'll need some more reading to have a informed opinion. My understanding of
> the Specials is still very high level.

In B3, we have either CheckValue or PatchpointValue. Instead of subclassing from StackmapValue, I have both of them have a Stackmap field inside them. You can think of Stackmap as "those things that are common between CheckValue and PatchpointValue". It's not clear that CheckValue and PatchpointValue should be separate classes, maybe we could fold Stackmap, CheckValue, and PatchpointValue into just StackmapValue. At some point I had things in PatchpointValue that I didn't have in StackmapValue, and after I removed those extra things, I still kept the separation because I was more interested in just completing the patch.

In Air, the patchpoints and checks get lowered to a Patch opcode with an Air::Special as its first operand. Air doesn't know anything about patchpoints, stackmaps, or checks. It only knows that a Patch instruction's behavior is determined by the virtual methods in Air::Special. When B3 lowers patchpoints, checks, etc to Air, it uses its own subclasses of Air::Special.

> 
> > > > Source/JavaScriptCore/b3/B3ValueRep.h:154
> > > > +    int64_t m_value;
> > > 
> > > I think this would look cleaner as an Union of Reg and intptr_t.
> > > union {
> > >     Reg register,
> > >     intptr_t offsetFromFP,
> > >     intptr_t offsetFromSP,
> > > } m_value;
> > 
> > But I can't have Reg in the union!
> 
> You can with C++11. It is used a bit in WebCore. You just need to be
> explicit about construction/destruction.

Interesting, I'll try it!

[Filip Pizlo]

Created attachment 264146 [details]
the patch

Added another immediate form handler in appendBinOp.  Switched B3::ValueRep to use a union.

[WebKit Commit Bot]

Attachment 264146 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:98:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:124:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 32 in 136 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #85)
> (In reply to comment #84)
> > (In reply to comment #78)
> > > > > Source/JavaScriptCore/b3/B3Stackmap.h:122
> > > > > +    RefPtr<Generator> m_generator;
> > > > 
> > > > I assume that's the Special's generator?
> > > > I find it a bit weird that it is on the StackMap. It may make more sense as
> > > > I read more of the context.
> > > 
> > > Where else would it be?  The idea is that the client that creates a
> > > patchpoint or CheckValue needs to supply a generator. 
> > 
> > I was expecting it on the value.
> > I'll need some more reading to have a informed opinion. My understanding of
> > the Specials is still very high level.
> 
> In B3, we have either CheckValue or PatchpointValue. Instead of subclassing
> from StackmapValue, I have both of them have a Stackmap field inside them.
> You can think of Stackmap as "those things that are common between
> CheckValue and PatchpointValue". It's not clear that CheckValue and
> PatchpointValue should be separate classes, maybe we could fold Stackmap,
> CheckValue, and PatchpointValue into just StackmapValue. At some point I had
> things in PatchpointValue that I didn't have in StackmapValue, and after I
> removed those extra things, I still kept the separation because I was more
> interested in just completing the patch.

Oh, I remember why I wanted PatchpointValue to be separate: patchpoints will require effectfulness annotations, while Check/CheckAdd/CheckMul/CheckSub are always effect-free.

[Benjamin Poulain]

Comment on attachment 264146 [details]
the patch

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

> Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:43
> +    explicit GenericFrequentedBlock(
> +        BasicBlock* block = nullptr, FrequencyClass frequency = FrequencyClass::Normal)

Could be on one line.

> Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:58
> +    bool operator==(const GenericFrequentedBlock& other) const
> +    {
> +        return m_block == other.m_block
> +            && m_frequency == other.m_frequency;
> +    }
> +
> +    bool operator!=(const GenericFrequentedBlock& other) const
> +    {
> +        return !(*this == other);
> +    }

I don't think you need that. Clang does it for you.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:68
> +        for (B3::BasicBlock* block : procedure)

The next loop goes over the block in pre-order.

Maybe it would be better to get the block list in pre-order, then allocate the Air block in that order. That way blocks used together are more likely to be allocated together.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:73
> +        for (B3::BasicBlock* block : procedure.blocksInPreOrder()) {

It's beautiful how simple this stage is. I hope it will stay like this when complexity adds up.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:109
> +

Everything after this could be private.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:153
> +        if (value->hasInt32())
> +            return Arg::imm(value->asInt32());

else if (value->hasInt64())
    return Arg::imm64(value->asInt64());

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:211
> +                // FIXME: Use the right Move opcode. If we're dealing with 32-bit types, we should
> +                // use Move32. If we're dealing with Doubles, we should use MoveDouble.
> +                // https://bugs.webkit.org/show_bug.cgi?id=150533

Use moveForType() and remove the FIXME?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:220
> +            append(Move, tmp(left), result);

Use moveForType()?

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:354
> +            selectedAddress = Arg::index(
> +                lower.tmp(base), lower.tmp(index), 1 << logScale->asInt());

Could be on a single line.

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:440
> +        switch (currentValue->type()) {

You could replace the switch with:
    append(moveForType(currentValue->type()), effectiveAddr(address), tmp(currentValue));

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:442
> +            append(Move32, effectiveAddr(address), tmp(currentValue));

In my opinion, it is easier to follow IR that use explicit Load/Store instead of move to/from addresses.

I'd split the MoveXX into Move-Load-Store.

> Source/JavaScriptCore/b3/B3MemoryValue.h:104
> +    MemoryValue(unsigned index, Opcode opcode, Origin origin, Value* pointer, int32_t offset = 0)

Isn't this always better than the previous form?
The explicit type argument looks error prone.

You could invoke the generic constructor from here instead to have some more precondition assertions for free.

> Source/JavaScriptCore/b3/B3MemoryValue.h:127
> +        : Value(index, opcode, Void, origin, value, pointer)

Ditto, you can invoke the main constructor instead.

> Source/JavaScriptCore/b3/B3Opcode.h:87
> +    SExt8,
> +    SExt16,
> +    // Takes Int32 and returns Int64:
> +    SExt32,

Tempted to write it as SexT8/16/32 for the lolz

> Source/JavaScriptCore/b3/B3Opcode.h:120
> +    LoadFloat,

LoadDouble?

> Source/JavaScriptCore/b3/B3Opcode.h:121
> +    // This return whatever the return type is:

is: ?

> Source/JavaScriptCore/b3/B3Opcode.h:129
> +    StoreFloat,

StoreDouble?

> Source/JavaScriptCore/b3/B3Procedure.cpp:48
> +    std::unique_ptr<BasicBlock> block(new BasicBlock(m_blocks.size(), frequency));

IIRC, Darin prefers:
    auto block = make_unique<>(...);

> Source/JavaScriptCore/b3/B3Procedure.h:88
> +        {

ASSERT(m_procedure == other.m_procedure);
?

> Source/JavaScriptCore/b3/B3Procedure.h:148
> +            {

ASSERT(m_procedure == other.m_procedure); ?

> Source/JavaScriptCore/b3/B3ProcedureInlines.h:39
> +    std::unique_ptr<ValueType> value(new ValueType(m_values.size(), arguments...));

auto, make_unique thingy

> Source/JavaScriptCore/b3/B3SuccessorCollection.h:58
> +            : m_collection(&collection) // const is stupid anyway.

You can remove the comment now.

> Source/JavaScriptCore/b3/B3SuccessorCollection.h:75
> +        {

ASSERT(m_collection == other.m_collection);

> Source/JavaScriptCore/b3/B3SwitchValue.h:89
> +        {

ASSERT(m_switch == other.m_switch);

> Source/JavaScriptCore/b3/B3UseCounts.cpp:40
> +            m_counts[child]++;

You may want to give Upsilons a useCount of 1 for consistency.

Not important with the current use of UseCounts.

> Source/JavaScriptCore/b3/B3Validate.cpp:84
> +                VALIDATE(
> +                    blocks.contains(successor), ("At ", *block, "->", pointerDump(successor)));

Could be on a single line.

> Source/JavaScriptCore/b3/B3Value.h:57
> +    // Note that the opcode is immutable, except for replacing values with identity.

identity or noop!

> Source/JavaScriptCore/b3/air/AirCode.cpp:47
> +    std::unique_ptr<BasicBlock> block(new BasicBlock(m_blocks.size(), frequency));

IIRC, Darin prefers:
    auto block = make_unique<>(...);

[Filip Pizlo]

*** Bug 150480 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

Created attachment 264185 [details]
the patch

I added a B3 validater.

[WebKit Commit Bot]

Attachment 264185 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:75:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:127:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:128:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:126:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:144:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:143:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:158:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 32 in 137 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

(In reply to comment #89)
> Comment on attachment 264146 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264146&action=review
> 
> > Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:43
> > +    explicit GenericFrequentedBlock(
> > +        BasicBlock* block = nullptr, FrequencyClass frequency = FrequencyClass::Normal)
> 
> Could be on one line.
> 
> > Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:58
> > +    bool operator==(const GenericFrequentedBlock& other) const
> > +    {
> > +        return m_block == other.m_block
> > +            && m_frequency == other.m_frequency;
> > +    }
> > +
> > +    bool operator!=(const GenericFrequentedBlock& other) const
> > +    {
> > +        return !(*this == other);
> > +    }
> 
> I don't think you need that. Clang does it for you.
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:68
> > +        for (B3::BasicBlock* block : procedure)
> 
> The next loop goes over the block in pre-order.
> 
> Maybe it would be better to get the block list in pre-order, then allocate
> the Air block in that order. That way blocks used together are more likely
> to be allocated together.

Pre-order walks are useful for when you want defs before uses. But the point of this lowering is for the Air CFG to be ordered the same way as the B3 CFG. That's why block creation is in-order.

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:73
> > +        for (B3::BasicBlock* block : procedure.blocksInPreOrder()) {
> 
> It's beautiful how simple this stage is. I hope it will stay like this when
> complexity adds up.
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:109
> > +
> 
> Everything after this could be private.

Almost did that, and then I realized that this is in an anonymous namespace and that if we added any other classes to the anonymous namespace, then they would all be friends of this one. ;-)

But I'll change it.

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:153
> > +        if (value->hasInt32())
> > +            return Arg::imm(value->asInt32());
> 
> else if (value->hasInt64())
>     return Arg::imm64(value->asInt64());

No.  The imm() method is for cases where the immediate can be an operand to an Air instruction.  If imm() returns the empty Arg(), it means that it's not possible to represent this Value* as an Arg::imm(). It would be wrong to return an Arg::imm64() here. Doing so would cause invalid lowering for things like this (taken from appendBinOp):

        if (imm(right) && isValidForm(opcode, Arg::Tmp, Arg::Imm, Arg::Tmp)) {
            append(opcode, tmp(left), imm(right), result);
            return;
        }

Notice how we're using imm(Value*) as a predicate, and if it returns non-empty, and the instruction supports a form that takes Arg::imm() for the second operand, then we emit such an instruction. This code would become incorrect if imm() could return an Arg::imm64().

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:211
> > +                // FIXME: Use the right Move opcode. If we're dealing with 32-bit types, we should
> > +                // use Move32. If we're dealing with Doubles, we should use MoveDouble.
> > +                // https://bugs.webkit.org/show_bug.cgi?id=150533
> 
> Use moveForType() and remove the FIXME?

Can't use moveForType().  We need something like coalescableMoveForType(), which uses Move instead of Move32 even for Int32.

If we used Move32 then the coalesce code would consider it off-limits since Move32 also does a zero-extend - so it's not a bit-for-bit move.

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:220
> > +            append(Move, tmp(left), result);
> 
> Use moveForType()?

Ditto.

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:354
> > +            selectedAddress = Arg::index(
> > +                lower.tmp(base), lower.tmp(index), 1 << logScale->asInt());
> 
> Could be on a single line.
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:440
> > +        switch (currentValue->type()) {
> 
> You could replace the switch with:
>     append(moveForType(currentValue->type()), effectiveAddr(address),
> tmp(currentValue));

Good call!

> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:442
> > +            append(Move32, effectiveAddr(address), tmp(currentValue));
> 
> In my opinion, it is easier to follow IR that use explicit Load/Store
> instead of move to/from addresses.
> 
> I'd split the MoveXX into Move-Load-Store.

That would break spilling. We want "Move %tmp1, %tmp2" to turn into "Move (%stack1), %tmp2" if we spill %tmp1.

Also, so long as math/compare/etc ops support addresses for operands, we're essentially acknowledging that Air is an orthogonal ISA (https://en.wikipedia.org/wiki/Orthogonal_instruction_set), then the accepted terminology for "Load/Store" is "Move".

> 
> > Source/JavaScriptCore/b3/B3MemoryValue.h:104
> > +    MemoryValue(unsigned index, Opcode opcode, Origin origin, Value* pointer, int32_t offset = 0)
> 
> Isn't this always better than the previous form?
> The explicit type argument looks error prone.

You need to supply it for Load.

> 
> You could invoke the generic constructor from here instead to have some more
> precondition assertions for free.

Good call.

> 
> > Source/JavaScriptCore/b3/B3MemoryValue.h:127
> > +        : Value(index, opcode, Void, origin, value, pointer)
> 
> Ditto, you can invoke the main constructor instead.

Done.

> 
> > Source/JavaScriptCore/b3/B3Opcode.h:87
> > +    SExt8,
> > +    SExt16,
> > +    // Takes Int32 and returns Int64:
> > +    SExt32,
> 
> Tempted to write it as SexT8/16/32 for the lolz

Haha!

> 
> > Source/JavaScriptCore/b3/B3Opcode.h:120
> > +    LoadFloat,
> 
> LoadDouble?

Load with a type() of Double.

> 
> > Source/JavaScriptCore/b3/B3Opcode.h:121
> > +    // This return whatever the return type is:
> 
> is: ?

Value::type().

> 
> > Source/JavaScriptCore/b3/B3Opcode.h:129
> > +    StoreFloat,
> 
> StoreDouble?

Store with child(0)->type() == Double.

> 
> > Source/JavaScriptCore/b3/B3Procedure.cpp:48
> > +    std::unique_ptr<BasicBlock> block(new BasicBlock(m_blocks.size(), frequency));
> 
> IIRC, Darin prefers:
>     auto block = make_unique<>(...);

I prefer it, too. But this causes Visual Studio build breakage because make_unique<> isn't granted permission to invoke BasicBlock's private constructor.  This is a convenient way of side-stepping the issue, and it's better than using friend or public.

> 
> > Source/JavaScriptCore/b3/B3Procedure.h:88
> > +        {
> 
> ASSERT(m_procedure == other.m_procedure);
> ?

Added.

> 
> > Source/JavaScriptCore/b3/B3Procedure.h:148
> > +            {
> 
> ASSERT(m_procedure == other.m_procedure); ?

Added.

> 
> > Source/JavaScriptCore/b3/B3ProcedureInlines.h:39
> > +    std::unique_ptr<ValueType> value(new ValueType(m_values.size(), arguments...));
> 
> auto, make_unique thingy

Ditto, ValueType has a private constructor and Visual Studio.

> 
> > Source/JavaScriptCore/b3/B3SuccessorCollection.h:58
> > +            : m_collection(&collection) // const is stupid anyway.
> 
> You can remove the comment now.

Yup, removed!

> 
> > Source/JavaScriptCore/b3/B3SuccessorCollection.h:75
> > +        {
> 
> ASSERT(m_collection == other.m_collection);

Added.

> 
> > Source/JavaScriptCore/b3/B3SwitchValue.h:89
> > +        {
> 
> ASSERT(m_switch == other.m_switch);

Added.

> 
> > Source/JavaScriptCore/b3/B3UseCounts.cpp:40
> > +            m_counts[child]++;
> 
> You may want to give Upsilons a useCount of 1 for consistency.
> 
> Not important with the current use of UseCounts.

Interesting.

> 
> > Source/JavaScriptCore/b3/B3Validate.cpp:84
> > +                VALIDATE(
> > +                    blocks.contains(successor), ("At ", *block, "->", pointerDump(successor)));
> 
> Could be on a single line.
> 
> > Source/JavaScriptCore/b3/B3Value.h:57
> > +    // Note that the opcode is immutable, except for replacing values with identity.
> 
> identity or noop!

Fixed.

> 
> > Source/JavaScriptCore/b3/air/AirCode.cpp:47
> > +    std::unique_ptr<BasicBlock> block(new BasicBlock(m_blocks.size(), frequency));
> 
> IIRC, Darin prefers:
>     auto block = make_unique<>(...);

BasicBlock has a private constructor and Visual Studio.

[Filip Pizlo]

FWIW, operator!= was not synthesized.  I will re-add all of the operator!='s that I removed per previous feedback.

[Filip Pizlo]

> > > Source/JavaScriptCore/b3/B3LowerToAir.cpp:109
> > > +
> > 
> > Everything after this could be private.
> 
> Almost did that, and then I realized that this is in an anonymous namespace
> and that if we added any other classes to the anonymous namespace, then they
> would all be friends of this one. ;-)
> 
> But I'll change it.

Nope, can't change it. The accept/try methods must be public.

I think that because private would have to be used very carefully in order to work, it's not worth it.

[Benjamin Poulain]

Comment on attachment 264185 [details]
the patch

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

> Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:134
> +    auto isRelevant = [] (const Arg& arg) -> bool {
> +        return arg.isStack() && arg.stackSlot()->kind() == StackSlotKind::Anonymous;
> +    };

This could be moved as a static function outside. The code is already pretty long.

isRelevant -> argumentIsOnStack?

> Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:140
> +    Liveness<Arg> liveness(code);
> +    IndexMap<StackSlot, HashSet<StackSlot*>> interference(code.stackSlots().size());
> +    Vector<StackSlot*> slots;
> +
> +    for (BasicBlock* block : code) {

I think you can this code out as "computeStackInterference()" that just produces the IndexMap.

[Benjamin Poulain]

Comment on attachment 264185 [details]
the patch

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

> Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:196
> +    Vector<StackSlot*> otherSlots;
> +    for (StackSlot* slot : code.stackSlots()) {

You could extract this loop as 
    assignStackOffsetToSlots()

> Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:205
> +        for (unsigned i = 0; i < assignedLockedStackSlots.size(); ++i)
> +            otherSlots[i] = assignedLockedStackSlots[i];

You could pre-assign those out of the loop.

> Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:215
> +    unsigned frameSizeForStackSlots = 0;
> +    for (StackSlot* slot : code.stackSlots()) {

extract in frameSizeForStackSlots()?

> Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:251
> +                        arg.offset() + arg.stackSlot()->offsetFromFP());

What's that arg.offset()?

> Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:68
> +    StackSlot* savesArea = code.addStackSlot(byteSize, StackSlotKind::Locked);
> +    // This is a bit weird since we could have already pinned a different stack slot to this
> +    // area. Also, our runtime does not require us to pin the saves area. Maybe we shouldn't pin it?
> +    savesArea->setOffsetFromFP(-byteSize);

I am a bit confused by this. 

Why allocate a big stackSlot at FP instead of allocating one StackSlot per register and letting the stack layout phase find them some space?

> Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:53
> +            RegisterSet& set = usedRegisters[block][index];

set -> registerSet

> Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:68
> +        for (unsigned instIndex = block->size(); instIndex--;) {

I think I would prefer to iterate over the Liveness<Tmp>::LocalCalc. The iterator would have
    iterator->inst
    iterator->live()

That way someone could use liveness analysis without knowing how it works.

Actually, you could make LocalCalc into the iterator and have something like:
    for (Liveness<Tmp>::LocalCalc localCalc : liveness.atBlock() {
        ...

> Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:92
> +            RegisterSet& setAfter = usedRegisters[block][instIndex + 1];

I am pretty confused by this.

If a register is not in setBefore, it is not live at the current instIndex. Why do you prevent the current instruction from using the register?

[Filip Pizlo]

(In reply to comment #96)
> Comment on attachment 264185 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264185&action=review
> 
> > Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:134
> > +    auto isRelevant = [] (const Arg& arg) -> bool {
> > +        return arg.isStack() && arg.stackSlot()->kind() == StackSlotKind::Anonymous;
> > +    };
> 
> This could be moved as a static function outside. The code is already pretty
> long.

OK.

> 
> isRelevant -> argumentIsOnStack?

OK.

> 
> > Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:140
> > +    Liveness<Arg> liveness(code);
> > +    IndexMap<StackSlot, HashSet<StackSlot*>> interference(code.stackSlots().size());
> > +    Vector<StackSlot*> slots;
> > +
> > +    for (BasicBlock* block : code) {
> 
> I think you can this code out as "computeStackInterference()" that just
> produces the IndexMap.

I kept it as is.  I think it's easier not to factor out things into functions if those functions are only called in one place.

[Filip Pizlo]

(In reply to comment #97)
> Comment on attachment 264185 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264185&action=review
> 
> > Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:196
> > +    Vector<StackSlot*> otherSlots;
> > +    for (StackSlot* slot : code.stackSlots()) {
> 
> You could extract this loop as 
>     assignStackOffsetToSlots()

I'll keep it here, since it's only used here.

> 
> > Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:205
> > +        for (unsigned i = 0; i < assignedLockedStackSlots.size(); ++i)
> > +            otherSlots[i] = assignedLockedStackSlots[i];
> 
> You could pre-assign those out of the loop.

OK.

> 
> > Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:215
> > +    unsigned frameSizeForStackSlots = 0;
> > +    for (StackSlot* slot : code.stackSlots()) {
> 
> extract in frameSizeForStackSlots()?

I'll keep it here.

> 
> > Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:251
> > +                        arg.offset() + arg.stackSlot()->offsetFromFP());
> 
> What's that arg.offset()?

Any use of a memory location has an offset.  That includes stack slots.  For example you can have a 64 byte stack slot named stack42 and access something like 16(stack42) - i.e. 16 bytes from the start of stack42.

> 
> > Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:68
> > +    StackSlot* savesArea = code.addStackSlot(byteSize, StackSlotKind::Locked);
> > +    // This is a bit weird since we could have already pinned a different stack slot to this
> > +    // area. Also, our runtime does not require us to pin the saves area. Maybe we shouldn't pin it?
> > +    savesArea->setOffsetFromFP(-byteSize);
> 
> I am a bit confused by this. 
> 
> Why allocate a big stackSlot at FP instead of allocating one StackSlot per
> register and letting the stack layout phase find them some space?

You could do it that way, but it would only make things more complicated and slower.

Since the callee-saves stack slot is live for the entire function, it's not any more efficient to allocate them individually.  It's less efficient if anything, since the compiler has more StackSlots to worry about in that case.

Also, no matter what we do, we need to be able to report to the client of B3 what the unwind data is - that is, where we have saved callee saves. The way I'm doing it right now means that we're reusing the baseline JIT's and DFG's callee save register layout code, and we're even reusing their data structure - the RegisterAtOffsetList.  Air::Code::calleeSaveRegisters() just returns that list. If the compiler could allocate callee-saves at any location after this phase runs, then we'd have to reconstruct the calleeSaveRegisters() list at the bitter end by someone tracking which StackSlot* corresponds to which callee save.  Sure, it's possible, but it strikes me as very roundabout.

> 
> > Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:53
> > +            RegisterSet& set = usedRegisters[block][index];
> 
> set -> registerSet

OK.

> 
> > Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:68
> > +        for (unsigned instIndex = block->size(); instIndex--;) {
> 
> I think I would prefer to iterate over the Liveness<Tmp>::LocalCalc. The
> iterator would have
>     iterator->inst
>     iterator->live()
> 
> That way someone could use liveness analysis without knowing how it works.

I don't think that's a good idea, and I don't see how to use this pattern correctly in this loop.  I think it's important for users of LocalCalc to *know* that they are iterating in reverse and that this is what the LocalCalc requires.  Notice that this use of LocalCalc has calls to setUsedRegisters() that uses Inst indices, and it actually uses the index of an instruction that is *different* from the one that liveness is going to operate on in addition to using the current one:

            Inst& inst = block->at(instIndex);
            setUsedRegisters(instIndex + 1, inst);
            localCalc.execute(inst);

It would be super weird to have such an index if we were using foreach iteration over the LocalCalc.

> 
> Actually, you could make LocalCalc into the iterator and have something like:
>     for (Liveness<Tmp>::LocalCalc localCalc : liveness.atBlock() {
>         ...

Maybe this would make sense here, but it's not a very general pattern.  Usually, you will use the liveness calc while performing some transformation that requires deliberately choosing some block iteration order.

> 
> > Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:92
> > +            RegisterSet& setAfter = usedRegisters[block][instIndex + 1];
> 
> I am pretty confused by this.
> 
> If a register is not in setBefore, it is not live at the current instIndex.
> Why do you prevent the current instruction from using the register?

Definitions. You could have an instruction like:

Move %tmp0, %tmp1

The problem is choosing a register to use for %tmp1. Lets say that %rax is dead before the Move. Does that mean we can use %rax for %tmp1? No, it doesn't. %rax could be live after. This would probably require a fairly exotic instruction - one that assigns to %rax directly even before register allocation. But that's totally something that we'll encounter. For example, the Intel div instruction will have to be modeled this way.

[Filip Pizlo]

*** Bug 150533 has been marked as a duplicate of this bug. ***

[Filip Pizlo]

Created attachment 264236 [details]
the patch

This patch includes the start of a strength reducer for B3.

[WebKit Commit Bot]

Attachment 264236 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:43:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:44:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:45:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:46:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:77:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:114:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:115:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:142:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:160:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:159:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3InsertionSet.h:63:  The parameter name "arguments" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 37 in 142 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 264243 [details]
the patch

More

[WebKit Commit Bot]

Attachment 264243 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:43:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:44:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:45:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:46:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:77:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:114:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:115:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:145:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:163:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/CMakeLists.txt:87:  Alphabetical sorting problem. "b3/B3ArgumentRegValue.cpp" should be before "b3/air/AirValidate.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/CMakeLists.txt:113:  Alphabetical sorting problem. "b3/B3StackSlotKind.cpp" should be before "b3/B3StackmapSpecial.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:159:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3InsertionSet.h:63:  The parameter name "arguments" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 39 in 148 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 264245 [details]
the patch

[WebKit Commit Bot]

Attachment 264245 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:43:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:44:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:45:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:46:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:77:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:114:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:115:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:145:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:163:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/CMakeLists.txt:87:  Alphabetical sorting problem. "b3/B3ArgumentRegValue.cpp" should be before "b3/air/AirValidate.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/CMakeLists.txt:113:  Alphabetical sorting problem. "b3/B3StackSlotKind.cpp" should be before "b3/B3StackmapSpecial.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:159:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3InsertionSet.h:63:  The parameter name "arguments" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 39 in 148 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Created attachment 264251 [details]
the patch

It's going to take some effort to make EWS happy - I added all of the B3+Air files to CMakeLists since we could do a CMake build on Darwin/x86_64 and then B3 would be enabled.  But as can be seen from the unhappy EWS bots, I've done a poor job of this.

[WebKit Commit Bot]

Attachment 264251 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/b3/air/AirInst.h:66:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/air/AirInst.h:111:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirGenerate.cpp:101:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:44:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3GenericFrequentedBlock.h:45:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3SwitchCase.h:32:  Alphabetical sorting problem.  [build/include_order] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:48:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:53:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:72:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirRegisterPriority.cpp:77:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:43:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:44:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:45:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3ConstDoubleValue.h:46:  The parameter name "proc" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:76:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:77:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:114:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3MemoryValue.h:115:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
ERROR: Source/JavaScriptCore/b3/B3Value.h:145:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/B3Value.h:163:  Missing space before {  [whitespace/braces] [5]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:73:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/testb3.cpp:87:  Consider using CHECK_EQ instead of CHECK(a == b)  [readability/check] [2]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:96:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:106:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirLiveness.h:113:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:52:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:62:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirSpillEverything.cpp:95:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/CMakeLists.txt:87:  Alphabetical sorting problem. "b3/B3ArgumentRegValue.cpp" should be before "b3/air/AirValidate.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/CMakeLists.txt:113:  Alphabetical sorting problem. "b3/B3StackSlotKind.cpp" should be before "b3/B3StackmapSpecial.cpp".  [list/order] [5]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:144:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirAllocateStack.cpp:159:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/air/AirHandleCalleeSaves.cpp:46:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3InsertionSet.h:63:  The parameter name "arguments" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/WTF/wtf/ScopedLambda.h:45:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/ScopedLambda.h:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/JavaScriptCore/b3/B3CheckSpecial.cpp:124:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/JavaScriptCore/b3/B3ControlValue.h:109:  Wrong number of spaces before statement. (expected: 12)  [whitespace/indent] [4]
Total errors found: 39 in 148 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Benjamin Poulain]

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

Let's land and iterate.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:81
> +            if (m_value->child(0)->opcode() == Add && isInt(m_value->type())) {

Comments would be good over each case of replaceWithNewValue. Finding the optimization by reading the conditions is non-trivial.

Alternatively, the pattern matcher could be useful here. You could have name for every optimization.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:127
> +                        address = memory->children().last() = address->child(0);

Should be two statements.

> Source/JavaScriptCore/b3/B3ReduceStrength.cpp:141
> +                    address = memory->children().last() = newAddress;

ditto

> Source/WTF/ChangeLog:16
> +

The changelog is not up to date...Not that I give a shit about file lists in ChangeLog :)

> Source/WTF/wtf/ListDump.h:58
> +    PointerListDump(const T& list, const char* comma)

comma -> separator in the whole class?

> Source/WTF/wtf/ListDump.h:66
> +        for (typename T::const_iterator iter = m_list.begin(); iter != m_list.end(); ++iter)

"auto" should be fine instead of "T::const_iterator". It would make the template more flexible.

> Source/WTF/wtf/ListDump.h:67
> +            out.print(m_comma, pointerDump(*iter));

Is it expected to put the comma/separator in front of the first value?

> Source/WTF/wtf/MathExtras.h:428
> +{

ASSERT(leftMin <= leftMax);
ASSERT(rightMin <= rightMax);

[Benjamin Poulain]

Comment on attachment 264251 [details]
the patch

Okay, let's land and iterate.

[Filip Pizlo]

(In reply to comment #109)
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=264236&action=review
> 
> Let's land and iterate.
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:81
> > +            if (m_value->child(0)->opcode() == Add && isInt(m_value->type())) {
> 
> Comments would be good over each case of replaceWithNewValue. Finding the
> optimization by reading the conditions is non-trivial.

Done.

> 
> Alternatively, the pattern matcher could be useful here. You could have name
> for every optimization.
> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:127
> > +                        address = memory->children().last() = address->child(0);
> 
> Should be two statements.

Done.

> 
> > Source/JavaScriptCore/b3/B3ReduceStrength.cpp:141
> > +                    address = memory->children().last() = newAddress;
> 
> ditto

Done.

> 
> > Source/WTF/ChangeLog:16
> > +
> 
> The changelog is not up to date...Not that I give a shit about file lists in
> ChangeLog :)

I'll update.

> 
> > Source/WTF/wtf/ListDump.h:58
> > +    PointerListDump(const T& list, const char* comma)
> 
> comma -> separator in the whole class?

We have this jargon in the ListDump and CommaPrinter that "comma" means "separator".  I'll keep it consistent for now.

> 
> > Source/WTF/wtf/ListDump.h:66
> > +        for (typename T::const_iterator iter = m_list.begin(); iter != m_list.end(); ++iter)
> 
> "auto" should be fine instead of "T::const_iterator". It would make the
> template more flexible.

Fixed.

> 
> > Source/WTF/wtf/ListDump.h:67
> > +            out.print(m_comma, pointerDump(*iter));
> 
> Is it expected to put the comma/separator in front of the first value?

CommaPrinter magically omits the first comma.

> 
> > Source/WTF/wtf/MathExtras.h:428
> > +{
> 
> ASSERT(leftMin <= leftMax);
> ASSERT(rightMin <= rightMax);

Done.

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/191705