1254142 - Baldr: Make control flow opcodes yield sub-expressions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: yield.patch

I remember saying to people "either it's gonna be really easy, or it's gonna be really hard". But thanks to some nice comments and ideas in pushJoinPredecessor (which I couldn't reuse; too bad), it happened to be very easy.

I've got another patch that shares more code with pushJoinPredecessor, but it's a bit bloated. Will submit for feedback though.

Comment 2

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Iterator POC for ensuring Pushed phis invariants

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Note that the first patch would break all the existing demos, because it introduces the fact that br_table has a value in all cases, the same way br/br_if have.

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1.yield.subexprs.patch

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2.iterator.poc.patch

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 8734879 [details] [diff] [review]
1.yield.subexprs.patch

Review of attachment 8734879 [details] [diff] [review]:
-----------------------------------------------------------------

Great!  Generally looks good to me, but a few questions:

::: js/src/asmjs/WasmIonCompile.cpp
@@ +1197,5 @@
>          if (!addControlFlowPatch(jump, relativeDepth, MGoto::TargetIndex))
>              return false;
>  
> +        if (maybeValue)
> +            push(curBlock_, maybeValue);

What's the rationale for using push() here instead of pushDef()?  It seems like with (call) you can have a MDefinition of type None.

@@ +1218,5 @@
>          if (!addControlFlowPatch(test, relativeDepth, MTest::TrueBranchIndex))
>              return false;
>  
> +        if (maybeValue)
> +            push(curBlock_, maybeValue);

ditto

@@ +1351,5 @@
> +        // the pending control flow instructions' owners (blocks) plus and
> +        // maybe the current block. Maintain the invariant that they've all
> +        // pushed, or none has. Do that before creating the join block, so that
> +        // it has the right number of stack slots.
> +        bool allPushed = !!def && (inDeadCode() || hasPushed(curBlock_));

I was wondering if this logic could be factored out and shared with addJoinPredecessor.  It might be easier if this logic was moved to addControlFlowPatch so that the invariant was maintained incrementally.

Comment 7

[Luke Wagner [:luke]]

Oops, I see you already noticed my last point and have a patch for it.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 8734879 [details] [diff] [review]
1.yield.subexprs.patch

Review of attachment 8734879 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/Wasm.cpp
@@ +610,5 @@
>      if (!f.d().readFixedU32(&defaultDepth))
>          return f.fail("expected default relative depth");
>  
> +    ExprType brType;
> +    if (!DecodeExpr(f, &brType))

Also: perhaps you you can split out the br_table changes into a separate patch so that we can land br_if now (since it's compatible with the current binary format and with v8).

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8734880 [details] [diff] [review]
2.iterator.poc.patch

Review of attachment 8734880 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/asmjs/WasmIonCompile.cpp
@@ +965,5 @@
> +
> +    struct BlockIter {
> +        virtual bool done() const = 0;
> +        virtual MBasicBlock* next() = 0;
> +        virtual void reset() = 0;

What if you made ensurePhiInvariants a template member function which took both the VectorT and also a GetBlockT which was called to get the block, given an index?  Then you could use C++ lambdas to elegantly pass these at the callsite.

Comment 10

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 1. Make Br/BrIf yield

Thank you for the feedback, all good points!

I've added two test cases that would have failed (br/br_if yielding MIRType_None).

I've also split the patch into two parts (part 2 is for br_table).

Even better, there's actually no need to use a template parameter if we use C++ lambdas and std::function typedefs!

Comment 11

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Make br_table yield

Feel free to cancel or defer review to whenever you think is the right time.

Comment 12

[Luke Wagner [:luke]]

Comment on attachment 8735839 [details] [diff] [review]
1. Make Br/BrIf yield

Review of attachment 8735839 [details] [diff] [review]:
-----------------------------------------------------------------

Good one with the lambdas!

::: js/src/asmjs/WasmIonCompile.cpp
@@ +959,4 @@
>          // either: every MBasicBlock has a non-void pushed expression OR no
>          // MBasicBlock has any pushed expression. This is required by
>          // MBasicBlock::addPredecessor.
> +        bool allPushed = !!def;

The meaning of 'def' here wasn't super obvious me; it seems to just be addressing the fact that loops' continue edge doesn't want to have anything pushed.  Instead of passing a null 'def' to the bindBranches in closeLoop, how about passing a dummy 'MDefinition* _;', removing the null checks in bindBranches, putting an "if (curBlock_ && hasPushed(curBlock_)) curBlock_->pop();" in closeLoop() after the bindBranches and then removing 'def' as a parameter here?

@@ +1329,5 @@
> +            if (def) {
> +                // Always pop if the curBlock pushed, but only assign if it's
> +                // non-void.
> +                *def = !inDeadCode() && hasPushed(curBlock_) ? curBlock_->pop() : nullptr;
> +                *def = *def && (*def)->type() != MIRType_None ? *def : nullptr;

IIUC, we always guard push() with a void-check, so pop() should always return a non-void and you can remove the second line and the comment.

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 8735840 [details] [diff] [review]
2. Make br_table yield

Review of attachment 8735840 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  I'll switch to r+ once we figure out the right time to land this wrt the demo update.

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Thank you for the review!

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/15a3458b4d11

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for bustage.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/be61ede92adb
Jobs with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=15a3458b4d114f6494370c5eae2583e77e6c73f5

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

So weird. I don't understand why there's a vanilla allocation here.

Can reproduce locally. If I remove the use of the std::function (or the mozilla equivalent, mozilla::function), I have no issues. Trying with a template parameter instead of the std::function / mozilla::function.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c32b93750409

Comment 18

[Benjamin Bouvier [:bbouvier] (inactive)]

Waldo: I have some code looking like this:

    template <typename GetBlock>
    void ensurePushInvariants(const GetBlock& getBlock, size_t numBlocks)
    {
       // for instance
       MBasicBlock* b = getBlock(numBlocks - 1);
    }

and the callers pass a lambda to this function:

    Vector<MBasicBlock*> blocks;
    /* stuff happens */
    auto getBlock = [&](size_t i) -> MBasicBlock* { return (*blocks)[i]; };
    ensurePushInvariants(getBlock, blocks->length());


I've initially figured out I could remove the template parameter with:

    typedef std::function<MBasicBlock*(size_t)> GetBlock;

and use this in ensurePushInvariants signature:

    void ensurePushInvariants(const GetBlock& getBlock, size_t numBlocks)

And code locally compiles, and everything is fine and the world is happy. Except that we can't use std::function because of |make check-cxx-something|. Fine, let's use mozilla:function, which has the same effect:

    typedef mozilla::function<MBasicBlock*(size_t)> GetBlock;

It compiles and runs fine on my machine. However, |make check-vanilla-allocations| doesn't pass, in this case:

TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'operator new(unsigned' present in Unified_cpp_js_src1.o
check_vanilla_allocations.py: Source lines with allocation calls:
check_vanilla_allocations.py: Accurate in unoptimized builds; jsutil.cpp expected.
check_vanilla_allocations.py: memalign called at /code/mozilla-inbound/js/src/jsutil.cpp:98
check_vanilla_allocations.py: operator new[](unsigned long) called at /code/mozilla-inbound/js/src/jsutil.cpp:97
check_vanilla_allocations.py: operator new(unsigned long) called at /code/mozilla-inbound/js/src/jsutil.cpp:94
check_vanilla_allocations.py: operator new(unsigned long) called at /home/ben/mozilla/builds/d64/dist/include/mozilla/Assertions.h:160

Any idea why? (it seems the mozilla::function uses raw new in the code, in contrary to some other mfbt stuff which seems to use new_ and variants, could that be the reason?)

I think I can land the patch with the template parameter in the meanwhile, but if you have any insights here, that would be helpful :-)

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c79a34d5b3a3

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c79a34d5b3a3

Comment 21

[Jeff Walden [:Waldo]]

I think over time mozilla::function has more or less moved in the direction of exactly emulating std::function.  So if that has raw news in it, mozilla::function would, too.  I'd just use the template version and live with it.

Comment 22

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Make br_table yield

rebased patch

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Make br_table yield

Rebased onto the postorder patch. If it's a straight r+ for this one, feel free to land at the same time as the postorder patch, when it gets r+'d.

Comment 25

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: 2. Make br_table yield

Tweaked to not take the yield value if the type is void, as a temporary workaround.

Comment 26

[Luke Wagner [:luke]]

Comment on attachment 8744385 [details] [diff] [review]
2. Make br_table yield

Review of attachment 8744385 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!  I'll take care of landing this as you suggested.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82274c64d408

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/82274c64d408