Bug 227510 – Add a new Air::Arg kind ZeroReg to let AIR recognise the new instructions/forms accepting zero register in ARM64

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yijia Huang]

...

[Radar WebKit Bug Importer]

<rdar://problem/80234128>

[Yijia Huang]

Created attachment 433174 [details]
Patch

[Yijia Huang]

Created attachment 433176 [details]
Patch

[Yijia Huang]

Created attachment 433181 [details]
Patch

[Yijia Huang]

Created attachment 433191 [details]
Patch

[Yijia Huang]

Created attachment 433192 [details]
Patch

[Yijia Huang]

Created attachment 433195 [details]
Patch

[Robin Morisset]

Comment on attachment 433195 [details]
Patch

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

> Source/JavaScriptCore/b3/B3LowerToAir.cpp:-1102
> -                    if (isValidForm(StoreZero32, dest.kind()) && dest.isValidForm(Width32))

Grepping for StoreZero32 and StoreZero64, I still see a lot of them in the code.
I think they should get removed and replaced by Store32/64 of Arg::ImmZero everywhere, rather than supporting both until the end of time.

> Source/JavaScriptCore/b3/air/AirArg.h:1057
> +        case ImmZero:

What is the reason for returning false here? Naively I would have expected this to return true since it can be used for integers.

[Saam Barati]

Comment on attachment 433195 [details]
Patch

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

>> Source/JavaScriptCore/b3/air/AirArg.h:1057
>> +        case ImmZero:
> 
> What is the reason for returning false here? Naively I would have expected this to return true since it can be used for integers.

Yeah, this should return true I believe

> Source/JavaScriptCore/b3/air/opcode_generator.rb:480
> +                                    parseError("Form has an zero register for a non-use argument")

nit: I'd use "zero immediate" instead of "zero register" here for consistency.

Maybe "Zero immediate must be a use"

> Source/JavaScriptCore/b3/air/opcode_generator.rb:483
> +                                    parseError("Form has an zero register for a non-general-purpose argument")

ditto, maybe:
"Zero immediate must be a general-purpose argument"

[Yijia Huang]

(In reply to Robin Morisset from comment #8)
> Comment on attachment 433195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433195&action=review
> 
> > Source/JavaScriptCore/b3/B3LowerToAir.cpp:-1102
> > -                    if (isValidForm(StoreZero32, dest.kind()) && dest.isValidForm(Width32))
> 
> Grepping for StoreZero32 and StoreZero64, I still see a lot of them in the
> code.
> I think they should get removed and replaced by Store32/64 of Arg::ImmZero
> everywhere, rather than supporting both until the end of time.

Thanks for the review. A previous bug report (https://bugs.webkit.org/show_bug.cgi?id=174821) already pointed out the problem that StoreZero is a temporary hack. I intend to remove all StoreZero references in this patch.

> > Source/JavaScriptCore/b3/air/AirArg.h:1057
> > +        case ImmZero:
> 
> What is the reason for returning false here? Naively I would have expected
> this to return true since it can be used for integers.

Will fix.

[Yijia Huang]

(In reply to Saam Barati from comment #9)
> Comment on attachment 433195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=433195&action=review
> 
> >> Source/JavaScriptCore/b3/air/AirArg.h:1057
> >> +        case ImmZero:
> > 
> > What is the reason for returning false here? Naively I would have expected this to return true since it can be used for integers.
> 
> Yeah, this should return true I believe
> 
> > Source/JavaScriptCore/b3/air/opcode_generator.rb:480
> > +                                    parseError("Form has an zero register for a non-use argument")
> 
> nit: I'd use "zero immediate" instead of "zero register" here for
> consistency.
> 
> Maybe "Zero immediate must be a use"
> 
> > Source/JavaScriptCore/b3/air/opcode_generator.rb:483
> > +                                    parseError("Form has an zero register for a non-general-purpose argument")
> 
> ditto, maybe:
> "Zero immediate must be a general-purpose argument"

Thanks. Will fix.

[Yijia Huang]

Created attachment 433227 [details]
Patch

[Saam Barati]

Comment on attachment 433227 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:8
> +        Add a new operand ImmZero to Air::Arg. So it can be recognized as the zero registers in ARM64 

"zero registers" => "zero register"

> Source/JavaScriptCore/ChangeLog:9
> +        Air opcode. In this case, the new overloads of the instructions accepting zero registers can 

ditto

> Source/JavaScriptCore/ChangeLog:11
> +

I'd also add some discussion here about how you're modeling it. e.g, modeling it like a special kind of "Imm", the "ZeroImm"

> Source/JavaScriptCore/b3/air/AirArg.h:91
> +        // ImmZero is recognized as a zero register for ARM64
> +        ImmZero

Nit, maybe "ZeroImm" for consistency with the other names, and I also think it reads a bit nicer.

> Source/JavaScriptCore/b3/air/AirOpcode.opcodes:681
> +arm64: Store32 U:G:32, D:G:32
> +    ImmZero, Addr
> +    ImmZero, Index
> +
> +arm64: Store64 U:G:64, D:G:64
> +    ImmZero, Addr
> +    ImmZero, Index

Were you going to remove the other instruction?

> Source/JavaScriptCore/b3/air/opcode_generator.rb:937
> +                when "ImmZero"

Why this here? When is this ruby code run? Why do we never do things for IsValidForm for ImmZero?

> Source/JavaScriptCore/b3/testb3_2.cpp:1307
> +void testMulNegArgArg(int a, int b)

why is this in this patch?

[Yijia Huang]

Created attachment 433294 [details]
Patch

[Yijia Huang]

Created attachment 433295 [details]
Patch

[Yijia Huang]

Created attachment 433298 [details]
Patch

[Saam Barati]

Comment on attachment 433298 [details]
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

[Yijia Huang]

Created attachment 433339 [details]
Patch

[Saam Barati]

Comment on attachment 433339 [details]
Patch

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

Seems like you introduced a lot of test failures.

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

should be false

> Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:160
> +                            RELEASE_ASSERT(isValidForm(Store32, Arg::ZeroReg, Arg::Stack));

was this run on x86 before? If so, gotta keep something around for x86. 

If so, could keep around StoreZero for x86.

[Yijia Huang]

Comment on attachment 433339 [details]
Patch

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

>> Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:160
>> +                            RELEASE_ASSERT(isValidForm(Store32, Arg::ZeroReg, Arg::Stack));
> 
> was this run on x86 before? If so, gotta keep something around for x86. 
> 
> If so, could keep around StoreZero for x86.

No.

[Yijia Huang]

Created attachment 433387 [details]
Patch

[Yijia Huang]

Created attachment 433390 [details]
Patch

[Yijia Huang]

Created attachment 433416 [details]
Patch

[Yijia Huang]

Created attachment 433418 [details]
Patch

[Yijia Huang]

Created attachment 433419 [details]
Patch

[Yijia Huang]

Created attachment 433420 [details]
Patch

[Yijia Huang]

Created attachment 433423 [details]
Patch

[Yijia Huang]

Created attachment 433427 [details]
Patch

[Saam Barati]

Comment on attachment 433427 [details]
Patch

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

r=me

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

"B3 and Air" => "Air"

> Source/JavaScriptCore/b3/air/AirLowerStackArgs.cpp:164
> +#elif CPU(X86_64) || CPU(X86)

I think you can just have CPU(X86_64) here

[Yijia Huang]

Created attachment 433444 [details]
Patch

[EWS]

Committed r279889 (239640@main): <https://commits.webkit.org/239640@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433444 [details].

[Yijia Huang]

*** Bug 174821 has been marked as a duplicate of this bug. ***