Closed Bug 1254142 Opened 4 years ago Closed 3 years ago

Baldr: Make control flow opcodes yield sub-expressions

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox47 --- affected
firefox49 --- fixed

People

(Reporter: bbouvier, Assigned: bbouvier)

References

Details

Attachments

(2 files, 7 obsolete files)

No description provided.
Attached patch yield.patch (obsolete) — Splinter Review
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.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8734877 - Flags: review?(luke)
Attachment #8734878 - Flags: feedback?(luke)
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.
Attached patch 1.yield.subexprs.patch (obsolete) — Splinter Review
Attachment #8734877 - Attachment is obsolete: true
Attachment #8734877 - Flags: review?(luke)
Attachment #8734879 - Flags: review?(luke)
Attached patch 2.iterator.poc.patch (obsolete) — Splinter Review
Attachment #8734878 - Attachment is obsolete: true
Attachment #8734878 - Flags: feedback?(luke)
Attachment #8734880 - Flags: feedback?(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.
Oops, I see you already noticed my last point and have a patch for it.
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 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.
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!
Attachment #8734879 - Attachment is obsolete: true
Attachment #8734879 - Flags: review?(luke)
Attachment #8735839 - Flags: review?(luke)
Attached patch 2. Make br_table yield (obsolete) — Splinter Review
Feel free to cancel or defer review to whenever you think is the right time.
Attachment #8734880 - Attachment is obsolete: true
Attachment #8734880 - Flags: feedback?(luke)
Attachment #8735840 - Flags: review?(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.
Attachment #8735839 - Flags: review?(luke) → review+
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.
Attachment #8735840 - Flags: review?(luke) → feedback+
Thank you for the review!
Keywords: leave-open
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
Flags: needinfo?(bbouvier)
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 :-)
Flags: needinfo?(jwalden+bmo)
Blocks: 1260256
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.
Flags: needinfo?(jwalden+bmo)
Attached patch 2. Make br_table yield (obsolete) — Splinter Review
rebased patch
Attachment #8735840 - Attachment is obsolete: true
Duplicate of this bug: 1266449
Attached patch 2. Make br_table yield (obsolete) — Splinter Review
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.
Attachment #8743407 - Attachment is obsolete: true
Attachment #8744282 - Flags: review?(sunfish)
Tweaked to not take the yield value if the type is void, as a temporary workaround.
Attachment #8744282 - Attachment is obsolete: true
Attachment #8744282 - Flags: review?(sunfish)
Attachment #8744385 - Flags: review?(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.
Attachment #8744385 - Flags: review?(luke) → review+
Keywords: leave-open
Blocks: 1263202
https://hg.mozilla.org/mozilla-central/rev/82274c64d408
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.