[webkit-reviews] review granted: [Bug 175396] Support compiling catch in the FTL : [Attachment 319312] patch

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 175396: Support compiling catch in the FTL
https://bugs.webkit.org/show_bug.cgi?id=175396

Attachment 319312: patch

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




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

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

r=me with comments

> JSTests/microbenchmarks/fake-iterators-that-throw-when-finished.js:76
> -    if (true)
> +    if (false)

lol

> Source/JavaScriptCore/b3/air/AirGenerate.cpp:226
> +		   jit.emitFunctionPrologue();
> +		   if (code.frameSize()) {
> +		       AllowMacroScratchRegisterUsageIf allowScratch(jit,
isARM64());
> +		      
jit.addPtr(CCallHelpers::TrustedImm32(-code.frameSize()),
MacroAssembler::stackPointerRegister);
> +		   }
> +		   
> +		   jit.emitSave(code.calleeSaveRegisterAtOffsetList());

Why not have Code set up default prologue generators that do this?

> Source/JavaScriptCore/dfg/DFGGraph.h:1014
> +	   // In SSA, this is the argument speculation that we've locked in for
an entrypoint block.
> +	   //
> +	   // We must speculate on the argument types at each entrypoint even
if operations involving
> +	   // arguments get killed. For example:
> +	   //
> +	   //	  function foo(x) {
> +	   //	     var tmp = x + 1;
> +	   //	  }
> +	   //
> +	   // Assume that x is always int during profiling. The ArithAdd for "x
+ 1" will be dead and will
> +	   // have a proven check for the edge to "x". So, we will not insert a
Check node and we will
> +	   // kill the GetStack for "x". But, we must do the int check in the
progolue, because that's the
> +	   // thing we used to allow DCE of ArithAdd. Otherwise the add could
be impure:
> +	   //
> +	   //	  var o = {
> +	   //	      valueOf: function() { do side effects; }
> +	   //	  };
> +	   //	  foo(o);
> +	   //
> +	   // If we DCE the ArithAdd and we remove the int check on x, then
this won't do the side
> +	   // effects.
> +	   //
> +	   // By convention, entrypoint index 0 is used for the CodeBlock's
op_enter entrypoint.
> +	   // So argumentFormats[0] are the argument formats for the normal
call entrypoint.
> +	   Vector<Vector<FlushFormat>> argumentFormats;
> +
> +	   // This maps an entrypoint index to a particular op_catch bytecode
offset. By convention,
> +	   // it'll never have zero as a key because we use zero to mean the
op_enter entrypoint.
> +	   HashMap<unsigned, unsigned> entrypointIndexToCatchBytecodeOffset;
> +
> +	   unsigned numberOfEntrypoints { UINT_MAX };
> +    };
> +    std::unique_ptr<SSAData> m_ssaData;

There's a lot of stuff in Graph that is SSA-only and it's not in a structure
like this.  Any reason why this can't just be directly on Graph?