Bug 227510 - Add a new Air::Arg kind ZeroReg to let AIR recognise the new instructions/forms accepting zero register in ARM64
Summary: Add a new Air::Arg kind ZeroReg to let AIR recognise the new instructions/for...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yijia Huang
URL:
Keywords: InRadar
: 174821 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-06-29 15:21 PDT by Yijia Huang
Modified: 2021-07-14 22:45 PDT (History)
10 users (show)

See Also:


Attachments
Patch (16.69 KB, patch)
2021-07-08 15:39 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.68 KB, patch)
2021-07-08 15:56 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.72 KB, patch)
2021-07-08 16:12 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (16.23 KB, patch)
2021-07-08 17:59 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (16.50 KB, patch)
2021-07-08 19:48 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (16.50 KB, patch)
2021-07-08 20:21 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (16.50 KB, patch)
2021-07-09 11:07 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (19.23 KB, patch)
2021-07-11 17:05 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (20.84 KB, patch)
2021-07-11 19:00 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (22.08 KB, patch)
2021-07-11 20:54 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (20.64 KB, patch)
2021-07-12 12:18 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (19.29 KB, patch)
2021-07-12 21:57 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (20.97 KB, patch)
2021-07-13 00:00 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (20.35 KB, patch)
2021-07-13 10:27 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.37 KB, patch)
2021-07-13 10:37 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.49 KB, patch)
2021-07-13 10:51 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.51 KB, patch)
2021-07-13 11:01 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.52 KB, patch)
2021-07-13 11:08 PDT, Yijia Huang
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (20.53 KB, patch)
2021-07-13 11:18 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff
Patch (20.52 KB, patch)
2021-07-13 12:27 PDT, Yijia Huang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yijia Huang 2021-06-29 15:21:07 PDT
...
Comment 1 Radar WebKit Bug Importer 2021-07-06 15:22:15 PDT
<rdar://problem/80234128>
Comment 2 Yijia Huang 2021-07-08 15:39:12 PDT
Created attachment 433174 [details]
Patch
Comment 3 Yijia Huang 2021-07-08 15:56:36 PDT
Created attachment 433176 [details]
Patch
Comment 4 Yijia Huang 2021-07-08 16:12:28 PDT
Created attachment 433181 [details]
Patch
Comment 5 Yijia Huang 2021-07-08 17:59:13 PDT
Created attachment 433191 [details]
Patch
Comment 6 Yijia Huang 2021-07-08 19:48:07 PDT
Created attachment 433192 [details]
Patch
Comment 7 Yijia Huang 2021-07-08 20:21:37 PDT
Created attachment 433195 [details]
Patch
Comment 8 Robin Morisset 2021-07-09 10:08:19 PDT
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.
Comment 9 Saam Barati 2021-07-09 10:34:53 PDT
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"
Comment 10 Yijia Huang 2021-07-09 10:48:24 PDT
(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.
Comment 11 Yijia Huang 2021-07-09 10:49:14 PDT
(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.
Comment 12 Yijia Huang 2021-07-09 11:07:45 PDT
Created attachment 433227 [details]
Patch
Comment 13 Saam Barati 2021-07-09 13:19:12 PDT
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?
Comment 14 Yijia Huang 2021-07-11 17:05:55 PDT
Created attachment 433294 [details]
Patch
Comment 15 Yijia Huang 2021-07-11 19:00:58 PDT
Created attachment 433295 [details]
Patch
Comment 16 Yijia Huang 2021-07-11 20:54:19 PDT
Created attachment 433298 [details]
Patch
Comment 17 Saam Barati 2021-07-12 11:19:22 PDT
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
Comment 18 Yijia Huang 2021-07-12 12:18:57 PDT
Created attachment 433339 [details]
Patch
Comment 19 Saam Barati 2021-07-12 20:40:59 PDT
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.
Comment 20 Yijia Huang 2021-07-12 21:34:13 PDT
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.
Comment 21 Yijia Huang 2021-07-12 21:57:36 PDT
Created attachment 433387 [details]
Patch
Comment 22 Yijia Huang 2021-07-13 00:00:34 PDT
Created attachment 433390 [details]
Patch
Comment 23 Yijia Huang 2021-07-13 10:27:34 PDT
Created attachment 433416 [details]
Patch
Comment 24 Yijia Huang 2021-07-13 10:37:27 PDT
Created attachment 433418 [details]
Patch
Comment 25 Yijia Huang 2021-07-13 10:51:30 PDT
Created attachment 433419 [details]
Patch
Comment 26 Yijia Huang 2021-07-13 11:01:40 PDT
Created attachment 433420 [details]
Patch
Comment 27 Yijia Huang 2021-07-13 11:08:50 PDT
Created attachment 433423 [details]
Patch
Comment 28 Yijia Huang 2021-07-13 11:18:31 PDT
Created attachment 433427 [details]
Patch
Comment 29 Saam Barati 2021-07-13 12:17:27 PDT
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
Comment 30 Yijia Huang 2021-07-13 12:27:01 PDT
Created attachment 433444 [details]
Patch
Comment 31 EWS 2021-07-13 13:38:27 PDT
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].
Comment 32 Yijia Huang 2021-07-14 22:45:51 PDT
*** Bug 174821 has been marked as a duplicate of this bug. ***