[webkit-reviews] review granted: [Bug 170215] WebAssembly: Air::Inst::generate crashes on large binary on A64 : [Attachment 309022] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted JF Bastien <jfbastien at apple.com>'s
request for review:
Bug 170215: WebAssembly: Air::Inst::generate crashes on large binary on A64
https://bugs.webkit.org/show_bug.cgi?id=170215

Attachment 309022: patch

https://bugs.webkit.org/attachment.cgi?id=309022&action=review




--- Comment #29 from Filip Pizlo <fpizlo at apple.com> ---
Comment on attachment 309022
  --> https://bugs.webkit.org/attachment.cgi?id=309022
patch

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

This is cool!  I like how this turned out.  Sorry I didn't get around to
reviewing it sooner.

> Source/JavaScriptCore/b3/B3Common.cpp:76
> +    return static_cast<GPRReg>(+MacroAssembler::dataTempRegister);

Why isn't dataTempRegister a GPRReg already?  I think that GPRReg is a typedef
for MacroAssembler::RegisterID.

> Source/JavaScriptCore/b3/air/AirCustom.h:90
> +    static bool admitsExtendedOffsetAddr(Inst& inst, unsigned argIndex)
> +    {
> +	   if (!argIndex)
> +	       return false;
> +	   return inst.args[0].special()->admitsExtendedOffsetAddr(inst,
argIndex);
> +    }
> +

You don't need to thread this through AirCustom if it's only for Patch.  You
could have Inst just assert that it's a Patch, or check if it is and return
false if it's not, and then do the special()->admintsBlahBlah thing directly.

However, this isn't bad.  I'll allow it. ;-)

> Source/JavaScriptCore/b3/air/opcode_generator.rb:232
> -    token =~
/\A((Tmp)|(Imm)|(BigImm)|(BitImm)|(BitImm64)|(SimpleAddr)|(Addr)|(Index)|(RelCo
nd)|(ResCond)|(DoubleCond)|(StatusCond))\Z/
> +    token =~
/\A((Tmp)|(Imm)|(BigImm)|(BitImm)|(BitImm64)|(SimpleAddr)|(Addr)|(ExtendedOffse
tAddr)|(Index)|(RelCond)|(ResCond)|(DoubleCond)|(StatusCond))\Z/

Again, don't really need to teach opcode_generator about it if it's just for
Patch.

But, this kind of generality isn't a bad thing and there's precedent for making
things a bit more general than strictly necessary, so it's OK to do it this
way.