[webkit-reviews] review granted: [Bug 170973] B3StackmapSpecial should handle when stackmap values are not recoverable from a Def'ed arg. : [Attachment 307446] proposed patch.

[bugzilla-daemon at webkit.org]

Filip Pizlo <fpizlo at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 170973: B3StackmapSpecial should handle when stackmap values are not
recoverable from a Def'ed arg.
https://bugs.webkit.org/show_bug.cgi?id=170973

Attachment 307446: proposed patch.

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




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

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

Have you measured perf?

r=me with suggested changes and perf results.  Octane is particularly sensitive
to CheckSpecial not using late unless it really has to.  If my guess is right,
this will be neutral on benchmarks - but that needs to be verified.

> Source/JavaScriptCore/b3/B3PatchpointSpecial.cpp:65
> +	   auto argWidth = inst.origin->resultWidth();
> +	   optionalDefArgWidth = argWidth;
> +	   callback(inst.args[argIndex++], role, inst.origin->resultBank(),
argWidth);
>      }
>  
> -    forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback);
> +    forEachArgImpl(0, argIndex, inst, SameAsRep, std::nullopt, callback,
optionalDefArgWidth);

I think that in Patchpoint, you don't need to do anything like this. 
Patchpoint does not have Check's weird recover-value-on-overflow behavior,
since it has no notion of overflow.  You should pass nullopt here.

> Source/JavaScriptCore/b3/B3StackmapSpecial.cpp:129
> +	       // If the Def'ed arg has a smaller width than the a stackmap
value, then we may not
> +	       // be able to recover the stackmap value. So, force LateColdUse
to preserve the
> +	       // original stackmap value across the Special operation.
> +	       if (optionalDefArgWidth && *optionalDefArgWidth <
child.value()->resultWidth())
> +		   role = Arg::LateColdUse;

I think that you want it to be a LateUse if it was a Use and a LateColdUse if
it was a ColdUse.  If it's already some kind of late use then you don't need to
change it.