Bug 162699 - Air should have a way of expressing additional instruction flags
Summary: Air should have a way of expressing additional instruction flags
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Filip Pizlo
URL:
Keywords:
Depends on:
Blocks: 162751 162689
  Show dependency treegraph
 
Reported: 2016-09-28 13:25 PDT by Filip Pizlo
Modified: 2016-10-10 19:55 PDT (History)
0 users

See Also:


Attachments
the patch (48.63 KB, patch)
2016-09-29 15:30 PDT, Filip Pizlo
mark.lam: review+
Details | Formatted Diff | Diff
the patch (48.59 KB, patch)
2016-09-30 09:49 PDT, Filip Pizlo
no flags Details | Formatted Diff | Diff
the patch (52.68 KB, patch)
2016-09-30 09:53 PDT, Filip Pizlo
mark.lam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Filip Pizlo 2016-09-28 13:25:00 PDT
Patch forthcoming.
Comment 1 Filip Pizlo 2016-09-28 13:25:19 PDT
This is like https://bugs.webkit.org/show_bug.cgi?id=162692, but it's not clear that we should do it the same way as in B3.
Comment 2 Filip Pizlo 2016-09-29 15:30:21 PDT
Created attachment 290255 [details]
the patch
Comment 3 Mark Lam 2016-09-30 09:41:07 PDT
Comment on attachment 290255 [details]
the patch

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

r=me with comments

> Source/JavaScriptCore/b3/air/AirInst.h:85
> +    

Please remove empty space.

> Source/JavaScriptCore/b3/air/AirKind.h:67
> +        return static_cast<unsigned>(opcode) + static_cast<unsigned>(traps);

Wouldn't this make it likely to have hash collisions between adjacent opcodes when flags are in use?  How about: "static_cast<unsigned>(opcode) + static_cast<unsigned>(traps) << 16" since sizeof Opcode is 16 bits?  Or is there a reason why collision doesn't matter?

> Source/JavaScriptCore/b3/air/opcode_generator.rb:1008
> +    outp.puts "if (kind.traps)"
> +    outp.puts "return true;"

Wish we could output pretty (indented) code. =(
Comment 4 Filip Pizlo 2016-09-30 09:48:31 PDT
(In reply to comment #3)
> Comment on attachment 290255 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290255&action=review
> 
> r=me with comments
> 
> > Source/JavaScriptCore/b3/air/AirInst.h:85
> > +    
> 
> Please remove empty space.

Done.

> 
> > Source/JavaScriptCore/b3/air/AirKind.h:67
> > +        return static_cast<unsigned>(opcode) + static_cast<unsigned>(traps);
> 
> Wouldn't this make it likely to have hash collisions between adjacent
> opcodes when flags are in use?  How about: "static_cast<unsigned>(opcode) +
> static_cast<unsigned>(traps) << 16" since sizeof Opcode is 16 bits?  Or is
> there a reason why collision doesn't matter?

Hashing the opcode or kind is super rare.  Currently the only client is B3::lowerToAir()'s HashMap that maps opcode/argument combinations to Specials.  I believe that this HashMap has at most 5 things in it ever!

But anyway your hash function is better, I'll use that.

> 
> > Source/JavaScriptCore/b3/air/opcode_generator.rb:1008
> > +    outp.puts "if (kind.traps)"
> > +    outp.puts "return true;"
> 
> Wish we could output pretty (indented) code. =(

I decided to uniformly give up on this because the generator emits recursively nested code, so it quickly loses track of how deep it is.  Instead, I print the code left-justified but in such a way that Emacs's indent-region makes the code look WebKit-style-approved.
Comment 5 Filip Pizlo 2016-09-30 09:49:44 PDT
Created attachment 290340 [details]
the patch
Comment 6 Filip Pizlo 2016-09-30 09:53:01 PDT
Created attachment 290342 [details]
the patch
Comment 7 Mark Lam 2016-09-30 09:58:43 PDT
Comment on attachment 290342 [details]
the patch

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

r=me with fix.

> Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:276
> +      ability to answer such questions. Fortunately, Air does not have much need to moving memory

/to moving/to move/.
Comment 8 Filip Pizlo 2016-09-30 09:59:19 PDT
(In reply to comment #7)
> Comment on attachment 290342 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=290342&action=review
> 
> r=me with fix.
> 
> > Websites/webkit.org/docs/b3/assembly-intermediate-representation.html:276
> > +      ability to answer such questions. Fortunately, Air does not have much need to moving memory
> 
> /to moving/to move/.

Thanks, fixed!
Comment 9 Filip Pizlo 2016-10-10 19:55:06 PDT
Landed in https://trac.webkit.org/changeset/206640