Bug 162699 – Air should have a way of expressing additional instruction flags

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

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.

[Filip Pizlo]

Created attachment 290255 [details]
the patch

[Mark Lam]

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. =(

[Filip Pizlo]

(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.

[Filip Pizlo]

Created attachment 290340 [details]
the patch

[Filip Pizlo]

Created attachment 290342 [details]
the patch

[Mark Lam]

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/.

[Filip Pizlo]

(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!

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/206640