Bug 170217 - WebAssembly: We should have Origins
Summary: WebAssembly: We should have Origins
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: Keith Miller
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-28 17:20 PDT by Keith Miller
Modified: 2017-03-29 11:03 PDT (History)
5 users (show)

See Also:


Attachments
Patch (91.55 KB, patch)
2017-03-28 17:24 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (92.33 KB, patch)
2017-03-28 17:56 PDT, Keith Miller
no flags Details | Formatted Diff | Diff
Patch for landing (93.30 KB, patch)
2017-03-28 21:51 PDT, Keith Miller
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2017-03-28 17:20:23 PDT
WebAssembly: We should have Origins
Comment 1 Keith Miller 2017-03-28 17:24:07 PDT
Created attachment 305681 [details]
Patch
Comment 2 Keith Miller 2017-03-28 17:25:43 PDT
Here's a sample:

BB#0: ; frequency = 1.000000
    Int64 @4 = Patchpoint(generator = 0x10f487fa8, earlyClobbered = [], lateClobbered = [], usedRegisters = [], resultConstraint = SomeRegister)
    Int64 @5 = FramePointer()
    Void @8 = Store(@4, @5, offset = 24, ControlDependent|Writes:Top)
    Int64 @10 = Const64(0)
    Void @12 = Store($0(@10), @5, offset = 16, ControlDependent|Writes:Top)
    Int64 @13 = Patchpoint(generator = 0x10f4be7f0, earlyClobbered = [], lateClobbered = [], usedRegisters = [], resultConstraint = SomeRegister, ExitsSideways|ControlDependent|WritesPinned|ReadsPinned|Fence|Writes:Top|Reads:Top)
    Int64 @16 = ArgumentReg(%rdi)
    Int64 @18 = ArgumentReg(%rsi)
    Int32 @22 = Trunc(@18, Wasm: {opcode: I64Rotl, location: 5})
    Int64 @23 = RotL(@16, @22, Wasm: {opcode: I64Rotl, location: 5})
    Void @27 = Return(@23, Terminal, Wasm: {opcode: End, location: 6})
Comment 3 Mark Lam 2017-03-28 17:38:47 PDT
Comment on attachment 305681 [details]
Patch

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

r=me if it builds.

> Source/JavaScriptCore/ChangeLog:9
> +        This patch adds wasm origins for B3::Values, called OpcodeOrigin. Currently,
> +        OpcodeOrigin just tracks the original opcode and the location of that opcode.

I think it would be nice to have the sample output after this paragraph.
Comment 4 Keith Miller 2017-03-28 17:56:39 PDT
Created attachment 305685 [details]
Patch for landing
Comment 5 Mark Lam 2017-03-28 21:02:39 PDT
Comment on attachment 305685 [details]
Patch for landing

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

> Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj:6299
> +				53C6FEF01E8AFE0C00B18425 /* WasmOpcodeOrigin.cpp */,

You need to add this to CMakeLists.txt as well.  That's what's ailing the GTK build.
Comment 6 Keith Miller 2017-03-28 21:51:21 PDT
Created attachment 305705 [details]
Patch for landing
Comment 7 WebKit Commit Bot 2017-03-28 22:32:49 PDT
Comment on attachment 305705 [details]
Patch for landing

Clearing flags on attachment: 305705

Committed r214529: <http://trac.webkit.org/changeset/214529>
Comment 8 WebKit Commit Bot 2017-03-28 22:32:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Saam Barati 2017-03-29 01:19:00 PDT
Comment on attachment 305705 [details]
Patch for landing

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

> Source/JavaScriptCore/wasm/WasmOpcodeOrigin.h:45
> +    const OpType opcode;

Can you pack this into a 64-bit word so we don't need to heap allocate these while compiling?
Comment 10 Keith Miller 2017-03-29 10:17:06 PDT
(In reply to Saam Barati from comment #9)
> Comment on attachment 305705 [details]
> Patch for landing
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305705&action=review
> 
> > Source/JavaScriptCore/wasm/WasmOpcodeOrigin.h:45
> > +    const OpType opcode;
> 
> Can you pack this into a 64-bit word so we don't need to heap allocate these
> while compiling?

I thought of this at one point but then I decided against it; I don't know why. I put a fix in: https://bugs.webkit.org/show_bug.cgi?id=170244
Comment 11 Saam Barati 2017-03-29 11:03:54 PDT
(In reply to Keith Miller from comment #10)
> (In reply to Saam Barati from comment #9)
> > Comment on attachment 305705 [details]
> > Patch for landing
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=305705&action=review
> > 
> > > Source/JavaScriptCore/wasm/WasmOpcodeOrigin.h:45
> > > +    const OpType opcode;
> > 
> > Can you pack this into a 64-bit word so we don't need to heap allocate these
> > while compiling?
> 
> I thought of this at one point but then I decided against it; I don't know
> why. I put a fix in: https://bugs.webkit.org/show_bug.cgi?id=170244
👍🏽