Bug 152165 - [JSC] Add an implementation of pow() taking an integer exponent to B3
Summary: [JSC] Add an implementation of pow() taking an integer exponent to B3
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Benjamin Poulain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-12-10 19:23 PST by Benjamin Poulain
Modified: 2015-12-12 22:12 PST (History)
2 users (show)

See Also:


Attachments
Patch (25.03 KB, patch)
2015-12-10 19:28 PST, Benjamin Poulain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Poulain 2015-12-10 19:23:42 PST
[JSC] Add an implementation of pow() taking an integer exponent to B3
Comment 1 Benjamin Poulain 2015-12-10 19:28:27 PST
Created attachment 267151 [details]
Patch
Comment 2 Mark Lam 2015-12-11 14:54:22 PST
Comment on attachment 267151 [details]
Patch

r=me.  Classic pow strength reduced to muls.  Nice.
Comment 3 WebKit Commit Bot 2015-12-11 15:45:26 PST
Comment on attachment 267151 [details]
Patch

Clearing flags on attachment: 267151

Committed r193989: <http://trac.webkit.org/changeset/193989>
Comment 4 WebKit Commit Bot 2015-12-11 15:45:30 PST
All reviewed patches have been landed.  Closing bug.
Comment 5 Filip Pizlo 2015-12-12 22:12:06 PST
Comment on attachment 267151 [details]
Patch

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

> Source/JavaScriptCore/b3/B3MathExtras.cpp:49
> +    BasicBlock* functionCallCase = procedure.addBlock();
> +    BasicBlock* loopPreHeaderCase = procedure.addBlock();
> +    BasicBlock* loopTestForEvenCase = procedure.addBlock();
> +    BasicBlock* loopOdd = procedure.addBlock();
> +    BasicBlock* loopEvenOdd = procedure.addBlock();
> +    BasicBlock* continuation = procedure.addBlock();

This breaks basic block ordering.  FTL creates all of the LLVM basic block entrypoints for DFG basic blocks, and then later it will insert basic blocks in between those other blocks so that all of the LLVM blocks associated with a DFG block are grouped together.  Right now, we have yet to port this feature from the LLVM version of Output to the B3 version.  But that does not mean that we will abandon the feature.  It's totally sensible for a compiler to assume that the incoming program order is appropriate for things like live ranges.

I don't see how to fix this given the restrictive API you've created.  Your API does not allow the caller to designate which block new blocks should be inserted before.  This is why FTL::Output works the way that it does.  Please remove the powDoubleInt32() function from the B3 namespace and move it to FTL::Output, so that once we port our basic block ordering code to B3, we can make it work with this.

> Source/JavaScriptCore/b3/B3MathExtras.h:41
> +// Raise "x" to "y" power.
> +// Return a new block continuing the flow and the value representing the result.
> +JS_EXPORT_PRIVATE std::pair<BasicBlock*, Value*> powDoubleInt32(Procedure&, BasicBlock*, Origin, Value* x, Value* y);

Let's not do this.  This belongs in FTLB3Output.  It's the wrong API, and it breaks the way that FTL orders basic blocks.  If you strongly feel that Pow(double, int) should be a B3 feature, then make it an opcode and handle it in LowerMacros.  But that doesn't seem like the right approach, since that just adds opcodes.

FTLOutput owns basic block ordering, which means that it has the power to implement an operation in terms of multiple basic blocks while still maintaining a sensible order.