[webkit-reviews] review denied: [Bug 227510] Add a new Air::Arg kind ImmZero to let AIR recognise the new instructions/forms accepting zero register in ARM64 : [Attachment 433298] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has denied Yijia Huang
<yijia_huang at apple.com>'s request for review:
Bug 227510: Add a new Air::Arg kind ImmZero to let AIR recognise the new
instructions/forms accepting zero register in ARM64
https://bugs.webkit.org/show_bug.cgi?id=227510

Attachment 433298: Patch

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




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

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

> Source/JavaScriptCore/ChangeLog:9
> +	   to different CPUs, the compiler must make instruction set details
explicit eventually. 

remove unicode character at the end

> Source/JavaScriptCore/ChangeLog:10
> +	   Then, Air is introduced, and it has syntax to speak of all of the
CPU instructions we 

I think here I'd just say: "Air is designed to target individual CPU
architectures and generate instructions specific to those CPUs" or similar.

> Source/JavaScriptCore/ChangeLog:11
> +	   know about. 

remove unicode character at the end

> Source/JavaScriptCore/ChangeLog:13
> +	   Previously, B3 and Air don't recognize the ZR register. This problem
has been pointed out 

don't => didn't
"ZR register" => "zero register"

> Source/JavaScriptCore/ChangeLog:18
> +	   should be introduced. Its goal is to closely match the CPU
instructions accepting a zero immediate 

"should be introduced" => "is introduced by this patch"

> Source/JavaScriptCore/ChangeLog:19
> +	   or the zero register in X86 and ARM64. Another reason is that with
this implementation, the new 

I don't think we're using the zero immediate for x86, right? I'd say this is
for arm64

> Source/JavaScriptCore/ChangeLog:22
> +	   Here, the ZeroImm is added as a new kind for Air::Arg, which acts as
a âhigh levelâ operand 

more unicode characters here

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:1108
> +		       if (isValidForm(Store32, Arg::ZeroImm, dest.kind()) &&
dest.isValidForm(Width32))
> +			   return Inst(Store32, m_value, zeroImm(), dest);

This code is already conditionalized under "isARM64" above. I'd just make these
instruction variants arm64 only inside AirOpcodes.opcodes

> Source/JavaScriptCore/b3/air/AirArg.h:67
> +	   // ZeroImm is interpreted as a zero register in ARM64 and
TrustedImm32(0) in X86 and X86_64

I would not make this a thing on x86. I'd just treat this as modeling how arm64
does things

> Source/JavaScriptCore/b3/air/AirArg.h:91
> +

remove

> Source/JavaScriptCore/b3/air/AirArg.h:760
> +	   case ZeroImm:

this is starting to feel a bit weird to me, and perhaps where the naming is
leading us to do weird things. I think I'd just call this the ZeroRegister.

> Source/JavaScriptCore/b3/air/AirArg.h:1264
> +	   case ZeroImm:
> +	       return isValidZeroImmForm(value());

I think this should just be return true, value() doesn't mean anything for
ZeroImm.

> Source/JavaScriptCore/b3/air/AirArg.h:1369
> +#elif CPU(X86_64) || CPU(X86)
> +    MacroAssembler::TrustedImm32 asZeroImm() const
> +    {
> +	   return MacroAssembler::TrustedImm32(0);
> +    }

ditto, I'd just assert at runtime we're arm64, and not have a concept of x86
here.

> Source/JavaScriptCore/b3/air/AirOpcode.opcodes:672
> +Store32 U:G:32, ZD:G:32
> +    ZeroImm, Addr
> +    ZeroImm, Index
>  
> -64: StoreZero64 D:G:64
> -    Addr
> -    Index
> +64: Store64 U:G:64, D:G:64
> +    ZeroImm, Addr
> +    ZeroImm, Index

I'd make these arm64 only