Reduce size of code emitted for stack loads/stores from Air on ARM #10263

eugeneia · 2023-02-17T11:08:28Z

`6d88e26`

Reduce size of code emitted for stack loads/stores from Air on ARM
https://bugs.webkit.org/show_bug.cgi?id=252469

Reviewed by Yusuke Suzuki.

This changes callFrameAddr (AirAllocateRegistersAndStackAndGenerateCode.cpp)
and its users to (which ever succeeds first)

 1. generate an address offset from GPRInfo::callFrameRegister
    (unchanged)
 2. generate an address offset from MacroAssembler::stackPointerRegister
 3. load the offset into extendedOffsetAddrRegister and generate
    a BaseIndex [GPRInfo::callFrameRegister, extendedOffsetAddrRegister]
 4. resort to computing the absolute address in extendedOffsetAddrRegister
    (this was the previous fallback after 1.)

For 3. to work reliably we need to change Air::Arg::isValidForm
to reject BaseIndex addressing when it is not supported on ARM.

The result of these changes is a significant reduction in code size
due to avoiding the fallback behavior in many cases (4).

Code generated for the JetStream2 WASM benchmarks is ~30% smaller on ARMv7
and ~40% smaller on ARM64.

* Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp:
(JSC::B3::Air::callFrameAddr):
(JSC::B3::Air::GenerateAndAllocateRegisters::flush):
(JSC::B3::Air::GenerateAndAllocateRegisters::alloc):
* Source/JavaScriptCore/b3/air/AirArg.h:
(JSC::B3::Air::Arg::isValidAddrForm):
(JSC::B3::Air::Arg::isValidForm const):

Canonical link: https://commits.webkit.org/260680@main

0a55d14

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	~~🧪 jsc-armv7-tests~~
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests
✅ 🛠 🧪 unsafe-merge

webkit-early-warning-system · 2023-02-17T11:26:25Z

EWS run on previous version of this PR (hash 7e54028)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	✅ 🧪 api-mac	~~🧪 gtk-wk2~~
	~~🧪 api-ios~~	✅ 🧪 mac-wk1	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	⏳ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	~~🧪 mac-AS-debug-wk2~~	⏳ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

webkit-early-warning-system · 2023-02-17T11:47:57Z

EWS run on previous version of this PR (hash 4909b66)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	✅ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	✅ 🧪 jsc-armv7-tests
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests

Constellation · 2023-02-19T21:07:29Z

Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp

+    if (addr.isValidForm(opcode, width))
+        return addr;
+
+    if (isARM64() || isARM_THUMB2()) {


We should not have arch condition here. If this is only allowed for these archs, then isValidForm should say false to wrong archs instead.

I removed those in da11283

Constellation · 2023-02-19T21:07:36Z

Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp

    GPRReg reg = extendedOffsetAddrRegister();
    jit.move(CCallHelpers::TrustedImmPtr(offsetFromFP), reg);
+    if (isARM64() || isARM_THUMB2()) {


Constellation · 2023-02-19T21:09:28Z

Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp

            ASSERT(m_map[tmp].spillSlot->byteSize() == bytesForWidth(Width64));
-            m_jit->loadDouble(callFrameAddr(Air::MoveDouble, *m_jit, offset), reg.fpr());
+            auto src = callFrameAddr(Air::MoveDouble, *m_jit, offset, m_code.frameSize(), Width64);
+            ASSERT(src.isAddr());


Can you explain why this is correct?

It used to be an invariant because this patch isValidForm rejected BaseIndex addresses for Air::MoveDouble. I've changed isValidIndexForm in da11283 to only reject Air::MoveDouble and Air::MoveFloat on ARM_THUMB2 (because the MacroAssemblerARMv7 will try to use the addressTempRegister to materialize them.) Hence, I've replaced the ASSERTs with branches to handle BaseIndex addresses for loadDouble/storeDouble/loadVector/storeVector (those can and do occur on ARM64.)

Constellation · 2023-02-19T21:09:34Z

Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp

        } else {
            ASSERT(m_map[tmp].spillSlot->byteSize() == bytesForWidth(Width128));
-            m_jit->loadVector(callFrameAddr(Air::MoveDouble, *m_jit, offset), reg.fpr());
+            auto src = callFrameAddr(Air::MoveDouble, *m_jit, offset, m_code.frameSize(), Width128);
+            ASSERT(src.isAddr());


Constellation · 2023-02-19T21:10:33Z

Source/JavaScriptCore/b3/air/AirArg.h

@@ -1396,7 +1412,7 @@ class Arg {
        case CallArg:
            return isValidAddrForm(opcode, offset(), width);
        case Index:
-            return isValidIndexForm(scale(), offset(), width);


isValidIndexForm should also take op ode instead

Fixed in da11283

Constellation · 2023-02-21T09:20:29Z

Code generated for the JetStream2 WASM benchmarks is ~30% smaller on ARMv7 and ~40% smaller on ARM64.

This is really great!

eugeneia · 2023-02-21T18:18:30Z

Do I have to do something to kick off a new EWS job for da11283? IIRC last time I force-pushed to this branch it just started a new job automatically.

tadeuzagallo · 2023-02-21T18:30:12Z

@eugeneia it should start automatically, but if it fails for some reason, you can force push again and normally it does the trick.

Constellation

r=me

Constellation · 2023-02-22T04:49:29Z

Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp

        } else {
            ASSERT(m_map[tmp].spillSlot->byteSize() == bytesForWidth(Width128));
-            m_jit->loadVector(callFrameAddr(Air::MoveDouble, *m_jit, offset), reg.fpr());
+            auto src = callFrameAddr(Air::MoveDouble, *m_jit, offset, m_code.frameSize(), Width128);


It looks like this is pre-existing issue, but we should use MoveVector (but it does not matter in terms of correctness I think).

Constellation · 2023-02-22T04:49:43Z

Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp

    } else {
        ASSERT(m_map[tmp].spillSlot->byteSize() == bytesForWidth(Width128));
-        m_jit->storeVector(reg.fpr(), callFrameAddr(Air::MoveDouble, *m_jit, offset));
+        auto dest = callFrameAddr(Air::MoveDouble, *m_jit, offset, m_code.frameSize(), Width128);


Constellation · 2023-02-22T04:56:04Z

Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp

+        return addr;
+
+    // Try finding a valid offset from the stack pointer register instead.
+    // (this trick is also done in Air::lowerStackArgs:126)


Let's remove this line since this line number etc. is fragile.

Constellation · 2023-02-22T05:28:24Z

Source/JavaScriptCore/b3/B3LowerToAir.cpp

@@ -571,7 +571,7 @@ class LowerToAir {
            Value* right = address->child(1);

            auto tryIndex = [&] (Value* index, Value* base) -> Arg {
-                std::optional<unsigned> scale = scaleForShl(index, offset, width);
+                std::optional<unsigned> scale = scaleForShl(Air::Move, index, offset, width);


Is Move correct?

I think so? I chose Air::Move because earlier in the function there is a comment:

// This function currently is currently only used for loads/stores, so // using Air::Move is appropriate.

(and a similar choice).

Not sure though. I find it tricky to do it these mappings from Air opcodes to desired isValidForm behavior.

Constellation · 2023-02-22T05:28:29Z

Source/JavaScriptCore/b3/B3LowerToAir.cpp

@@ -598,7 +598,7 @@ class LowerToAir {
            // amount is greater than 1, then there isn't really anything smart that we could do here.
            // We avoid using baseless indexes because their encoding isn't particularly efficient.
            if (m_locked.contains(left) || !address->child(1)->isInt32(1)
-                || !Arg::isValidIndexForm(1, offset, width))
+                || !Arg::isValidIndexForm(Air::Move, 1, offset, width))


Is Move correct?

Constellation · 2023-02-22T05:28:32Z

Source/JavaScriptCore/b3/B3LowerToAir.cpp

@@ -613,7 +613,7 @@ class LowerToAir {
        case WasmAddress: {
            WasmAddressValue* wasmAddress = address->as<WasmAddressValue>();
            Value* pointer = wasmAddress->child(0);
-            if (!Arg::isValidIndexForm(1, offset, width) || m_locked.contains(pointer))
+            if (!Arg::isValidIndexForm(Air::Move, 1, offset, width) || m_locked.contains(pointer))


Is Move correct?

Constellation · 2023-02-22T05:28:42Z

Source/JavaScriptCore/b3/B3LowerToAir.cpp

@@ -2325,7 +2325,7 @@ class LowerToAir {
        }

        auto tryShl = [&] (Value* shl, Value* other) -> bool {
-            std::optional<unsigned> scale = scaleForShl(shl, offset);
+            std::optional<unsigned> scale = scaleForShl(leaOpcode, shl, offset);


Is leaOpcode correct?

Again, I think so? Not sure what the other candidates would be?

webkit-early-warning-system · 2023-02-22T11:29:33Z

EWS run on previous version of this PR (hash b17b385)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
❌ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	⏳ 🛠 wpe	⏳ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	⏳ 🧪 ios-wk2	⏳ 🧪 api-mac	~~🧪 gtk-wk2~~
	⏳ 🧪 api-ios	~~🧪 mac-wk1~~	~~🧪 api-gtk~~
⏳ 🛠 🧪 jsc	✅ 🛠 tv	~~🧪 mac-wk2~~	⏳ 🛠 jsc-armv7
~~🛠 🧪 jsc-arm64~~	✅ 🛠 tv-sim	⏳ 🧪 mac-AS-debug-wk2	⏳ 🧪 jsc-armv7-tests
	⏳ 🛠 watch	~~🧪 mac-wk2-stress~~	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		~~🧪 jsc-mips-tests~~

webkit-early-warning-system · 2023-02-22T11:38:19Z

EWS run on current version of this PR (hash 0a55d14)

Misc	iOS, tvOS & watchOS	macOS	Linux	Windows
✅ 🧪 style	✅ 🛠 ios	✅ 🛠 mac	✅ 🛠 wpe	✅ 🛠 wincairo
	✅ 🛠 ios-sim	✅ 🛠 mac-AS-debug	✅ 🛠 gtk
✅ 🧪 webkitperl	✅ 🧪 ios-wk2	✅ 🧪 api-mac	✅ 🧪 gtk-wk2
	✅ 🧪 api-ios	✅ 🧪 mac-wk1	❌ 🧪 api-gtk
✅ 🛠 🧪 jsc	✅ 🛠 tv	✅ 🧪 mac-wk2	✅ 🛠 jsc-armv7
✅ 🛠 🧪 jsc-arm64	✅ 🛠 tv-sim	✅ 🧪 mac-AS-debug-wk2	~~🧪 jsc-armv7-tests~~
	✅ 🛠 watch	✅ 🧪 mac-wk2-stress	✅ 🛠 jsc-mips
	✅ 🛠 watch-sim		✅ 🧪 jsc-mips-tests
✅ 🛠 🧪 unsafe-merge

https://bugs.webkit.org/show_bug.cgi?id=252469 Reviewed by Yusuke Suzuki. This changes callFrameAddr (AirAllocateRegistersAndStackAndGenerateCode.cpp) and its users to (which ever succeeds first) 1. generate an address offset from GPRInfo::callFrameRegister (unchanged) 2. generate an address offset from MacroAssembler::stackPointerRegister 3. load the offset into extendedOffsetAddrRegister and generate a BaseIndex [GPRInfo::callFrameRegister, extendedOffsetAddrRegister] 4. resort to computing the absolute address in extendedOffsetAddrRegister (this was the previous fallback after 1.) For 3. to work reliably we need to change Air::Arg::isValidForm to reject BaseIndex addressing when it is not supported on ARM. The result of these changes is a significant reduction in code size due to avoiding the fallback behavior in many cases (4). Code generated for the JetStream2 WASM benchmarks is ~30% smaller on ARMv7 and ~40% smaller on ARM64. * Source/JavaScriptCore/b3/air/AirAllocateRegistersAndStackAndGenerateCode.cpp: (JSC::B3::Air::callFrameAddr): (JSC::B3::Air::GenerateAndAllocateRegisters::flush): (JSC::B3::Air::GenerateAndAllocateRegisters::alloc): * Source/JavaScriptCore/b3/air/AirArg.h: (JSC::B3::Air::Arg::isValidAddrForm): (JSC::B3::Air::Arg::isValidForm const): Canonical link: https://commits.webkit.org/260680@main

webkit-commit-queue · 2023-02-22T18:18:30Z

Committed 260680@main (6d88e26): https://commits.webkit.org/260680@main

Reviewed commits have been landed. Closing PR #10263 and removing active labels.

eugeneia requested a review from a team as a code owner February 17, 2023 11:08

eugeneia force-pushed the max-bias-v1-squash branch from 22f6fb0 to 7e54028 Compare February 17, 2023 11:10

eugeneia force-pushed the max-bias-v1-squash branch from 7e54028 to 4909b66 Compare February 17, 2023 11:47

Constellation requested changes Feb 19, 2023

View reviewed changes

eugeneia force-pushed the max-bias-v1-squash branch from 4909b66 to da11283 Compare February 21, 2023 18:07

Constellation approved these changes Feb 22, 2023

View reviewed changes

Constellation reviewed Feb 22, 2023

View reviewed changes

eugeneia force-pushed the max-bias-v1-squash branch from da11283 to b17b385 Compare February 22, 2023 11:28

eugeneia force-pushed the max-bias-v1-squash branch from b17b385 to 0a55d14 Compare February 22, 2023 11:37

Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 22, 2023

webkit-early-warning-system force-pushed the max-bias-v1-squash branch from 0a55d14 to 6d88e26 Compare February 22, 2023 18:18

webkit-early-warning-system merged commit 6d88e26 into WebKit:main Feb 22, 2023

webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Feb 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce size of code emitted for stack loads/stores from Air on ARM #10263

Reduce size of code emitted for stack loads/stores from Air on ARM #10263

eugeneia commented Feb 17, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 17, 2023 •

edited

webkit-early-warning-system commented Feb 17, 2023 •

edited

Constellation Feb 19, 2023

eugeneia Feb 21, 2023

Constellation Feb 19, 2023

Constellation Feb 19, 2023

eugeneia Feb 21, 2023

Constellation Feb 19, 2023

Constellation Feb 19, 2023

eugeneia Feb 21, 2023

Constellation commented Feb 21, 2023

eugeneia commented Feb 21, 2023

tadeuzagallo commented Feb 21, 2023

Constellation left a comment

Constellation Feb 22, 2023

Constellation Feb 22, 2023

Constellation Feb 22, 2023

Constellation Feb 22, 2023

eugeneia Feb 22, 2023

Constellation Feb 22, 2023

eugeneia Feb 22, 2023

Constellation Feb 22, 2023

eugeneia Feb 22, 2023

Constellation Feb 22, 2023

eugeneia Feb 22, 2023

webkit-early-warning-system commented Feb 22, 2023 •

edited

webkit-early-warning-system commented Feb 22, 2023 •

edited

webkit-commit-queue commented Feb 22, 2023

Reduce size of code emitted for stack loads/stores from Air on ARM #10263

Reduce size of code emitted for stack loads/stores from Air on ARM #10263

Conversation

eugeneia commented Feb 17, 2023 • edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 17, 2023 • edited

webkit-early-warning-system commented Feb 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Constellation commented Feb 21, 2023

eugeneia commented Feb 21, 2023

tadeuzagallo commented Feb 21, 2023

Constellation left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

webkit-early-warning-system commented Feb 22, 2023 • edited

webkit-early-warning-system commented Feb 22, 2023 • edited

webkit-commit-queue commented Feb 22, 2023

eugeneia commented Feb 17, 2023 •

edited by webkit-early-warning-system

webkit-early-warning-system commented Feb 17, 2023 •

edited

webkit-early-warning-system commented Feb 17, 2023 •

edited

webkit-early-warning-system commented Feb 22, 2023 •

edited

webkit-early-warning-system commented Feb 22, 2023 •

edited