[webkit-reviews] review denied: [Bug 201703] Elide unnecessary moves in Air O0 : [Attachment 378701] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Keith Miller
<keith_miller at apple.com>'s request for review:
Bug 201703: Elide unnecessary moves in Air O0
https://bugs.webkit.org/show_bug.cgi?id=201703

Attachment 378701: Patch

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




--- Comment #10 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 378701
  --> https://bugs.webkit.org/attachment.cgi?id=378701
Patch

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

> Source/JavaScriptCore/ChangeLog:14
> +	   This appears to be a minor progression on the overall score of
> +	   wasm subtests in JS2 and a 10% wasm-JIT memory usage reduction.

these results still hold?

>
Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:43
8
> +		   Reg reg = source.isReg() ? source.reg() :
m_map[source.tmp()].reg;

nit: let's call this sourceReg

Also, this looks wrong. What if the value supposed to be in $rax is actually
spilled? Don't you always want to consult m_map? An instruction may ask for a
named register, but it doesn't mean the corresponding value is already in that
register.

If you read allocNamed below, you'll see the problem. We might load, say, $rax,
from where it is on the stack back into $rax. That is ignored here.

>
Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:-6
19
> -		   bool needsToGenerate = true;

can you assert it's false here? Personally, I'd just keep this variable, since
they're kinda unrelated