[M] Change in dart/sdk[main]: [vm] Unify `LoadInt32FromBoxOrSmi` in assemblers

[Daco Harkes (Gerrit)]

Attention is currently required from: Tess Strickland.

Daco Harkes would like Tess Strickland to review this change.

View Change

[vm] Unify `LoadInt32FromBoxOrSmi` in assemblers

And add unit tests.

TEST=vm/cc/Assembler_LoadInt32

Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de
Cq-Include-Trybots: luci.dart.try:vm-linux-release-simarm64-try,vm-aot-linux-release-simarm64-try,vm-linux-debug-x64c-try,vm-aot-linux-debug-simriscv64-try
---
M runtime/vm/compiler/assembler/assembler_arm.h
M runtime/vm/compiler/assembler/assembler_arm64.h
M runtime/vm/compiler/assembler/assembler_ia32.h
M runtime/vm/compiler/assembler/assembler_riscv.h
M runtime/vm/compiler/assembler/assembler_test.cc
M runtime/vm/compiler/assembler/assembler_x64.cc
M runtime/vm/compiler/assembler/assembler_x64.h
M runtime/vm/compiler/backend/il_arm64.cc
M runtime/vm/compiler/backend/il_ia32.cc
M runtime/vm/compiler/backend/il_x64.cc
10 files changed, 85 insertions(+), 50 deletions(-)

diff --git a/runtime/vm/compiler/assembler/assembler_arm.h b/runtime/vm/compiler/assembler/assembler_arm.h
index 6f1d084..67309cb 100644
--- a/runtime/vm/compiler/assembler/assembler_arm.h
+++ b/runtime/vm/compiler/assembler/assembler_arm.h
@@ -1376,6 +1376,7 @@
     LoadInt32FromBoxOrSmi(result, value);
   }
 
+  // Truncates upper bits.
   void LoadInt32FromBoxOrSmi(Register result, Register value) {
     if (result == value) {
       ASSERT(TMP != value);
diff --git a/runtime/vm/compiler/assembler/assembler_arm64.h b/runtime/vm/compiler/assembler/assembler_arm64.h
index a250757..8a286f3 100644
--- a/runtime/vm/compiler/assembler/assembler_arm64.h
+++ b/runtime/vm/compiler/assembler/assembler_arm64.h
@@ -1747,6 +1747,23 @@
     LoadInt64FromBoxOrSmi(result, value);
   }
 
+  // Truncates upper bits.
+  void LoadInt32FromBoxOrSmi(Register result, Register value) {
+    if (result == value) {
+      ASSERT(TMP != value);
+      MoveRegister(TMP, value);
+      value = TMP;
+    }
+    ASSERT(value != result);
+    compiler::Label done;
+    sbfx(result, value, kSmiTagSize,
+         Utils::Minimum(static_cast<int>(32), compiler::target::kSmiBits));
+    BranchIfSmi(value, &done);
+    LoadFieldFromOffset(result, value, compiler::target::Mint::value_offset(),
+                        compiler::kFourBytes);
+    Bind(&done);
+  }
+
   void LoadInt64FromBoxOrSmi(Register result, Register value) {
     if (result == value) {
       ASSERT(TMP != value);
diff --git a/runtime/vm/compiler/assembler/assembler_ia32.h b/runtime/vm/compiler/assembler/assembler_ia32.h
index 6a5993b..f0de3a0 100644
--- a/runtime/vm/compiler/assembler/assembler_ia32.h
+++ b/runtime/vm/compiler/assembler/assembler_ia32.h
@@ -1079,6 +1079,7 @@
     LoadInt32FromBoxOrSmi(result, value);
   }
 
+  // Truncates upper bits.
   void LoadInt32FromBoxOrSmi(Register result, Register value) {
     if (result != value) {
       MoveRegister(result, value);
diff --git a/runtime/vm/compiler/assembler/assembler_riscv.h b/runtime/vm/compiler/assembler/assembler_riscv.h
index 15471a5..984154b 100644
--- a/runtime/vm/compiler/assembler/assembler_riscv.h
+++ b/runtime/vm/compiler/assembler/assembler_riscv.h
@@ -966,6 +966,7 @@
 #endif
   }
 
+  // Truncates upper bits.
   void LoadInt32FromBoxOrSmi(Register result, Register value) {
     if (result == value) {
       ASSERT(TMP != value);
diff --git a/runtime/vm/compiler/assembler/assembler_test.cc b/runtime/vm/compiler/assembler/assembler_test.cc
index 92aff00..fe97ff4 100644
--- a/runtime/vm/compiler/assembler/assembler_test.cc
+++ b/runtime/vm/compiler/assembler/assembler_test.cc
@@ -147,19 +147,19 @@
 #define REG2 CallingConventions::ArgumentRegisters[1]
 #endif
 
-#define LOAD_FROM_BOX_TEST(VALUE, SAME_REGISTER)                               \
-  ASSEMBLER_TEST_GENERATE(LoadWordFromBoxOrSmi##VALUE##SAME_REGISTER,          \
+#define LOAD_FROM_BOX_TEST(SIZE, TYPE, VALUE, SAME_REGISTER)                   \
+  ASSEMBLER_TEST_GENERATE(Load##SIZE##FromBoxOrSmi##VALUE##SAME_REGISTER,      \
                           assembler) {                                         \
     const bool same_register = SAME_REGISTER;                                  \
                                                                                \
     const Register src = REG1;                                                 \
     const Register dst = same_register ? src : REG2;                           \
-    const intptr_t value = VALUE;                                              \
+    const TYPE value = VALUE;                                                  \
                                                                                \
     EnterTestFrame(assembler);                                                 \
                                                                                \
     __ LoadObject(src, Integer::ZoneHandle(Integer::New(value, Heap::kOld)));  \
-    __ LoadWordFromBoxOrSmi(dst, src);                                         \
+    __ Load##SIZE##FromBoxOrSmi(dst, src);                                     \
     __ MoveRegister(CallingConventions::kReturnReg, dst);                      \
                                                                                \
     LeaveTestFrame(assembler);                                                 \
@@ -167,29 +167,40 @@
     __ Ret();                                                                  \
   }                                                                            \
                                                                                \
-  ASSEMBLER_TEST_RUN(LoadWordFromBoxOrSmi##VALUE##SAME_REGISTER, test) {       \
+  ASSEMBLER_TEST_RUN(Load##SIZE##FromBoxOrSmi##VALUE##SAME_REGISTER, test) {   \
     const int64_t res = test->InvokeWithCodeAndThread<int64_t>();              \
-    EXPECT_EQ(static_cast<intptr_t>(VALUE), static_cast<intptr_t>(res));       \
+    EXPECT_EQ(static_cast<TYPE>(VALUE), static_cast<TYPE>(res));               \
   }
 
-LOAD_FROM_BOX_TEST(0, true)
-LOAD_FROM_BOX_TEST(0, false)
-LOAD_FROM_BOX_TEST(1, true)
-LOAD_FROM_BOX_TEST(1, false)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0, true)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0, false)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 1, true)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 1, false)
 #if defined(TARGET_ARCH_IS_32_BIT)
-LOAD_FROM_BOX_TEST(0x7FFFFFFF, true)
-LOAD_FROM_BOX_TEST(0x7FFFFFFF, false)
-LOAD_FROM_BOX_TEST(0x80000000, true)
-LOAD_FROM_BOX_TEST(0x80000000, false)
-LOAD_FROM_BOX_TEST(0xFFFFFFFF, true)
-LOAD_FROM_BOX_TEST(0xFFFFFFFF, false)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0x7FFFFFFF, true)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0x7FFFFFFF, false)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0x80000000, true)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0x80000000, false)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0xFFFFFFFF, true)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0xFFFFFFFF, false)
 #else
-LOAD_FROM_BOX_TEST(0x7FFFFFFFFFFFFFFF, true)
-LOAD_FROM_BOX_TEST(0x7FFFFFFFFFFFFFFF, false)
-LOAD_FROM_BOX_TEST(0x8000000000000000, true)
-LOAD_FROM_BOX_TEST(0x8000000000000000, false)
-LOAD_FROM_BOX_TEST(0xFFFFFFFFFFFFFFFF, true)
-LOAD_FROM_BOX_TEST(0xFFFFFFFFFFFFFFFF, false)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0x7FFFFFFFFFFFFFFF, true)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0x7FFFFFFFFFFFFFFF, false)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0x8000000000000000, true)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0x8000000000000000, false)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0xFFFFFFFFFFFFFFFF, true)
+LOAD_FROM_BOX_TEST(Word, intptr_t, 0xFFFFFFFFFFFFFFFF, false)
 #endif
 
+LOAD_FROM_BOX_TEST(Int32, int32_t, 0, true)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 0, false)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 1, true)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 1, false)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 0x7FFFFFFF, true)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 0x7FFFFFFF, false)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 0x80000000, true)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 0x80000000, false)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 0xFFFFFFFF, true)
+LOAD_FROM_BOX_TEST(Int32, int32_t, 0xFFFFFFFF, false)
+
 }  // namespace dart
diff --git a/runtime/vm/compiler/assembler/assembler_x64.cc b/runtime/vm/compiler/assembler/assembler_x64.cc
index f12a9c0..7e536b3 100644
--- a/runtime/vm/compiler/assembler/assembler_x64.cc
+++ b/runtime/vm/compiler/assembler/assembler_x64.cc
@@ -1351,6 +1351,33 @@
   Bind(&done);
 }
 
+void Assembler::LoadInt32FromBoxOrSmi(Register result, Register value) {
+  compiler::Label done;
+#if !defined(DART_COMPRESSED_POINTERS)
+  // Optimistically untag value.
+  SmiUntag(result, value);
+  j(NOT_CARRY, &done, compiler::Assembler::kNearJump);
+  // Undo untagging by multiplying value by 2.
+  // [reg + reg + disp8] has a shorter encoding than [reg*2 + disp32]
+  movsxd(result, compiler::Address(result, result, TIMES_1,
+                                   compiler::target::Mint::value_offset()));
+#else
+  if (result == value) {
+    ASSERT(TMP != value);
+    MoveRegister(TMP, value);
+    value = TMP;
+  }
+  ASSERT(value != result);
+  // Cannot speculatively untag with value == result because it erases the
+  // upper bits needed to dereference when it is a Mint.
+  SmiUntagAndSignExtend(result, value);
+  j(NOT_CARRY, &done, compiler::Assembler::kNearJump);
+  movsxd(result,
+         compiler::FieldAddress(value, compiler::target::Mint::value_offset()));
+#endif
+  Bind(&done);
+}
+
 void Assembler::LoadIsolate(Register dst) {
   movq(dst, Address(THR, target::Thread::isolate_offset()));
 }
diff --git a/runtime/vm/compiler/assembler/assembler_x64.h b/runtime/vm/compiler/assembler/assembler_x64.h
index 8d3fa8f..67c14f2 100644
--- a/runtime/vm/compiler/assembler/assembler_x64.h
+++ b/runtime/vm/compiler/assembler/assembler_x64.h
@@ -1076,6 +1076,9 @@
     LoadInt64FromBoxOrSmi(result, value);
   }
 
+  // Truncates upper bits.
+  void LoadInt32FromBoxOrSmi(Register result, Register value);
+
   void LoadInt64FromBoxOrSmi(Register result, Register value);
 
   void BranchIfNotSmi(Register reg,
diff --git a/runtime/vm/compiler/backend/il_arm64.cc b/runtime/vm/compiler/backend/il_arm64.cc
index 60801df..8fdf75f 100644
--- a/runtime/vm/compiler/backend/il_arm64.cc
+++ b/runtime/vm/compiler/backend/il_arm64.cc
@@ -3636,14 +3636,7 @@
 void UnboxInstr::EmitLoadInt32FromBoxOrSmi(FlowGraphCompiler* compiler) {
   const Register value = locs()->in(0).reg();
   const Register result = locs()->out(0).reg();
-  ASSERT(value != result);
-  compiler::Label done;
-  __ sbfx(result, value, kSmiTagSize,
-          Utils::Minimum(static_cast<intptr_t>(32), kSmiBits));
-  __ BranchIfSmi(value, &done);
-  __ LoadFieldFromOffset(result, value, Mint::value_offset(),
-                         compiler::kFourBytes);
-  __ Bind(&done);
+  __ LoadInt32FromBoxOrSmi(result, value);
 }
 
 void UnboxInstr::EmitLoadInt64FromBoxOrSmi(FlowGraphCompiler* compiler) {
diff --git a/runtime/vm/compiler/backend/il_ia32.cc b/runtime/vm/compiler/backend/il_ia32.cc
index 6123567..f4d8e08 100644
--- a/runtime/vm/compiler/backend/il_ia32.cc
+++ b/runtime/vm/compiler/backend/il_ia32.cc
@@ -3573,7 +3573,6 @@
 void UnboxInstr::EmitLoadInt32FromBoxOrSmi(FlowGraphCompiler* compiler) {
   const Register value = locs()->in(0).reg();
   const Register result = locs()->out(0).reg();
-  ASSERT(value == result);
   __ LoadInt32FromBoxOrSmi(result, value);
 }
 
diff --git a/runtime/vm/compiler/backend/il_x64.cc b/runtime/vm/compiler/backend/il_x64.cc
index 473c358..b552056 100644
--- a/runtime/vm/compiler/backend/il_x64.cc
+++ b/runtime/vm/compiler/backend/il_x64.cc
@@ -3865,25 +3865,7 @@
 void UnboxInstr::EmitLoadInt32FromBoxOrSmi(FlowGraphCompiler* compiler) {
   const Register value = locs()->in(0).reg();
   const Register result = locs()->out(0).reg();
-  compiler::Label done;
-#if !defined(DART_COMPRESSED_POINTERS)
-  ASSERT(value == result);
-  // Optimistically untag value.
-  __ SmiUntag(value);
-  __ j(NOT_CARRY, &done, compiler::Assembler::kNearJump);
-  // Undo untagging by multiplying value by 2.
-  // [reg + reg + disp8] has a shorter encoding than [reg*2 + disp32]
-  __ movsxd(result,
-            compiler::Address(value, value, TIMES_1, Mint::value_offset()));
-#else
-  ASSERT(value != result);
-  // Cannot speculatively untag with value == result because it erases the
-  // upper bits needed to dereference when it is a Mint.
-  __ SmiUntagAndSignExtend(result, value);
-  __ j(NOT_CARRY, &done, compiler::Assembler::kNearJump);
-  __ movsxd(result, compiler::FieldAddress(value, Mint::value_offset()));
-#endif
-  __ Bind(&done);
+  __ LoadInt32FromBoxOrSmi(result, value);
 }
 
 void UnboxInstr::EmitLoadInt64FromBoxOrSmi(FlowGraphCompiler* compiler) {

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: newchange

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 3

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Tess Strickland <sstr...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Tess Strickland <sstr...@google.com>

[Daco Harkes (Gerrit)]

Attention is currently required from: Tess Strickland.

View Change

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 3

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Tess Strickland <sstr...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Tess Strickland <sstr...@google.com>

Gerrit-Comment-Date: Thu, 28 Sep 2023 13:34:27 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No

[Tess Strickland (Gerrit)]

Attention is currently required from: Daco Harkes.

Patch set 3:Code-Review +1

View Change

3 comments:

• Patchset:

  • Patch Set #3:

    LGTM outside nits.

• File runtime/vm/compiler/assembler/assembler_arm64.h:

  • Patch Set #3, Line 1751: LoadInt32FromBoxOrSmi

    Nit: Perhaps we should add virtual declarations to `assembler_base.h`:
    ```
      void LoadInt32FromBoxOrSmi(Register result, Register value) = 0;
      void LoadWordFromBoxOrSmi(Register result, Register value) = 0;
    #if !defined(TARGET_ARCH_IS_32_BITS)
      void LoadInt64FromBoxOrSmi(Register result, Register value) = 0;
    #endif
    ```
    and override them in the arch-specific assemblers. This way, it's clear which architectures have which of these methods.

    Note that if we add the `LoadInt64FromBoxOrSmi` one, we'll need to guard the definition of `LoadInt64FromBoxOrSmi` in `assembler_riscv.h` with 
    ```
    #if !defined(TARGET_ARCH_IS_32_BITS)
      void LoadInt64FromBoxOrSmi(Register result, Register value) override {
        ...
      }
    #endif
    ```

• File runtime/vm/compiler/assembler/assembler_x64.cc:

  • Patch Set #3, Line 1360: // Undo untagging by multiplying value by 2.

    Change this comment to match however you change the comment for the IA32 version in the previous CL in the chain (since there's no multiplication by 2).

    Also consider adding
    ```
      COMPILE_ASSERT(kSmiTagShift == 1);
    ```
    for the same reasons as the parent CL.  (Also in `LoadInt64FromBoxOrSmi` too, up to you).

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 3

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Tess Strickland <sstr...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Daco Harkes <dacoh...@google.com>

Gerrit-Comment-Date: Fri, 29 Sep 2023 11:45:28 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Daco Harkes (Gerrit)]

Attention is currently required from: Daco Harkes.

Daco Harkes uploaded patch set #4 to this change.

View Change

[vm] Unify `LoadInt32FromBoxOrSmi` in assemblers

And add unit tests.

TEST=vm/cc/Assembler_LoadInt32

Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de
Cq-Include-Trybots: luci.dart.try:vm-linux-release-simarm64-try,vm-aot-linux-release-simarm64-try,vm-linux-debug-x64c-try,vm-aot-linux-debug-simriscv64-try
---
M runtime/vm/compiler/assembler/assembler_arm.h
M runtime/vm/compiler/assembler/assembler_arm64.h

M runtime/vm/compiler/assembler/assembler_base.h

M runtime/vm/compiler/assembler/assembler_ia32.h
M runtime/vm/compiler/assembler/assembler_riscv.h
M runtime/vm/compiler/assembler/assembler_test.cc
M runtime/vm/compiler/assembler/assembler_x64.cc
M runtime/vm/compiler/assembler/assembler_x64.h
M runtime/vm/compiler/backend/il_arm64.cc
M runtime/vm/compiler/backend/il_ia32.cc
M runtime/vm/compiler/backend/il_x64.cc

11 files changed, 104 insertions(+), 77 deletions(-)

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: newpatchset

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 4

[Daco Harkes (Gerrit)]

View Change

3 comments:

• Patchset:

  • Patch Set #4:

    Thanks Tess!

• File runtime/vm/compiler/assembler/assembler_arm64.h:

• • Patch Set #3, Line 1751: LoadInt32FromBoxOrSmi

• • Nit: Perhaps we should add virtual declarations to `assembler_base.h`: […]

    Done

• File runtime/vm/compiler/assembler/assembler_x64.cc:

• • Patch Set #3, Line 1360: // Undo untagging by multiplying value by 2.

• • Change this comment to match however you change the comment for the IA32 version in the previous CL […]

    Done

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 4

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Tess Strickland <sstr...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Fri, 29 Sep 2023 12:55:49 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: No

Comment-In-Reply-To: Tess Strickland <sstr...@google.com>

[Tess Strickland (Gerrit)]

Attention is currently required from: Daco Harkes.

Patch set 4:Code-Review +1

View Change

1 comment:

• File runtime/vm/compiler/assembler/assembler_riscv.h:

  • Patch Set #4, Line 977: void LoadInt64FromBoxOrSmi(Register result, Register value) override {

    You need to `#ifdef` this as I suggested, because RISCV32 exists (and we don't have a trybot for it AFAIK, so you'll have to test building by hand).

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 4

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Tess Strickland <sstr...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Daco Harkes <dacoh...@google.com>

Gerrit-Comment-Date: Fri, 29 Sep 2023 13:04:26 +0000

Gerrit-HasComments: Yes

Gerrit-Has-Labels: Yes

[Tess Strickland (Gerrit)]

Attention is currently required from: Daco Harkes.

View Change

1 comment:

• File runtime/vm/compiler/assembler/assembler_riscv.h:

• • Patch Set #4, Line 977: void LoadInt64FromBoxOrSmi(Register result, Register value) override {

• • You need to `#ifdef` this as I suggested, because RISCV32 exists (and we don't have a trybot for it […]

    That does mean you can remove the `#if XLEN == 32` block in the body though.

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 4

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Tess Strickland <sstr...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Attention: Daco Harkes <dacoh...@google.com>

Gerrit-Comment-Date: Fri, 29 Sep 2023 13:05:01 +0000

[Daco Harkes (Gerrit)]

Attention is currently required from: Daco Harkes.

Daco Harkes uploaded patch set #5 to this change.

View Change

[vm] Unify `LoadInt32FromBoxOrSmi` in assemblers

And add unit tests.

TEST=vm/cc/Assembler_LoadInt32

Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de
Cq-Include-Trybots: luci.dart.try:vm-linux-release-simarm64-try,vm-aot-linux-release-simarm64-try,vm-linux-debug-x64c-try,vm-aot-linux-debug-simriscv64-try
---
M runtime/vm/compiler/assembler/assembler_arm.h
M runtime/vm/compiler/assembler/assembler_arm64.h
M runtime/vm/compiler/assembler/assembler_base.h
M runtime/vm/compiler/assembler/assembler_ia32.h
M runtime/vm/compiler/assembler/assembler_riscv.h
M runtime/vm/compiler/assembler/assembler_test.cc
M runtime/vm/compiler/assembler/assembler_x64.cc
M runtime/vm/compiler/assembler/assembler_x64.h
M runtime/vm/compiler/backend/il_arm64.cc
M runtime/vm/compiler/backend/il_ia32.cc
M runtime/vm/compiler/backend/il_x64.cc

11 files changed, 107 insertions(+), 80 deletions(-)

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: newpatchset

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 5

[Daco Harkes (Gerrit)]

View Change

2 comments:

• Patchset:

  • Patch Set #5:

    Nice catch Tess!

• File runtime/vm/compiler/assembler/assembler_riscv.h:

• • Patch Set #4, Line 977: void LoadInt64FromBoxOrSmi(Register result, Register value) override {

• • That does mean you can remove the `#if XLEN == 32` block in the body though.

    Nice catch!

    I did a local build for simriscv32 now.

    `$ tools/build.py -mrelease -asimriscv32 run_vm_tests runtime && tools/test.py -n vm-linux-release-simriscv32 vm/cc/Assembler_Load`

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 5

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Tess Strickland <sstr...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-CC: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Comment-Date: Fri, 29 Sep 2023 13:20:48 +0000

[Daco Harkes (Gerrit)]

Daco Harkes uploaded patch set #6 to this change.

View Change

[vm] Unify `LoadInt32FromBoxOrSmi` in assemblers

And add unit tests.

TEST=vm/cc/Assembler_LoadInt32

Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de
Cq-Include-Trybots: luci.dart.try:vm-linux-release-simarm64-try,vm-aot-linux-release-simarm64-try,vm-linux-debug-x64c-try,vm-aot-linux-debug-simriscv64-try
---
M runtime/vm/compiler/assembler/assembler_arm.h
M runtime/vm/compiler/assembler/assembler_arm64.h
M runtime/vm/compiler/assembler/assembler_base.h
M runtime/vm/compiler/assembler/assembler_ia32.h
M runtime/vm/compiler/assembler/assembler_riscv.h
M runtime/vm/compiler/assembler/assembler_test.cc
M runtime/vm/compiler/assembler/assembler_x64.cc
M runtime/vm/compiler/assembler/assembler_x64.h
M runtime/vm/compiler/backend/il_arm64.cc
M runtime/vm/compiler/backend/il_ia32.cc
M runtime/vm/compiler/backend/il_x64.cc

11 files changed, 106 insertions(+), 81 deletions(-)

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: newpatchset

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 6

[Commit Queue (Gerrit)]

Commit Queue submitted this change.

View Change

4 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:

```
The name of the file: runtime/vm/compiler/assembler/assembler_riscv.h
Insertions: 2, Deletions: 4.

@@ -974,23 +974,21 @@
     Bind(&done);
   }
 
+#if XLEN != 32

   void LoadInt64FromBoxOrSmi(Register result, Register value) override {

     if (result == value) {
       ASSERT(TMP != value);

       MoveRegister(TMP, value);
       value = TMP;
     }
-#if XLEN == 32
-    UNIMPLEMENTED();
-#else
     ASSERT(value != result);
     compiler::Label done;
     SmiUntag(result, value);
     BranchIfSmi(value, &done, compiler::Assembler::kNearJump);
     LoadFieldFromOffset(result, value, target::Mint::value_offset());
     Bind(&done);
-#endif
   }
+#endif
 
   void BranchIfNotSmi(Register reg,
                       Label* label,
```

Approvals: Tess Strickland: Looks good to me, approved

[vm] Unify `LoadInt32FromBoxOrSmi` in assemblers

And add unit tests.

TEST=vm/cc/Assembler_LoadInt32

Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de
Cq-Include-Trybots: luci.dart.try:vm-linux-release-simarm64-try,vm-aot-linux-release-simarm64-try,vm-linux-debug-x64c-try,vm-aot-linux-debug-simriscv64-try

Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/328521
Reviewed-by: Tess Strickland <sstr...@google.com>

---
M runtime/vm/compiler/assembler/assembler_arm.h
M runtime/vm/compiler/assembler/assembler_arm64.h
M runtime/vm/compiler/assembler/assembler_base.h
M runtime/vm/compiler/assembler/assembler_ia32.h
M runtime/vm/compiler/assembler/assembler_riscv.h
M runtime/vm/compiler/assembler/assembler_test.cc
M runtime/vm/compiler/assembler/assembler_x64.cc
M runtime/vm/compiler/assembler/assembler_x64.h
M runtime/vm/compiler/backend/il_arm64.cc
M runtime/vm/compiler/backend/il_ia32.cc
M runtime/vm/compiler/backend/il_x64.cc
11 files changed, 106 insertions(+), 81 deletions(-)

diff --git a/runtime/vm/compiler/assembler/assembler_arm.h b/runtime/vm/compiler/assembler/assembler_arm.h
index 6f1d084..df99544 100644
--- a/runtime/vm/compiler/assembler/assembler_arm.h
+++ b/runtime/vm/compiler/assembler/assembler_arm.h
@@ -1372,11 +1372,8 @@
     b(label, NE);
   }
 
-  void LoadWordFromBoxOrSmi(Register result, Register value) {
-    LoadInt32FromBoxOrSmi(result, value);
-  }
-
-  void LoadInt32FromBoxOrSmi(Register result, Register value) {

+  // Truncates upper bits.

+  void LoadInt32FromBoxOrSmi(Register result, Register value) override {

     if (result == value) {
       ASSERT(TMP != value);

       MoveRegister(TMP, value);
diff --git a/runtime/vm/compiler/assembler/assembler_arm64.h b/runtime/vm/compiler/assembler/assembler_arm64.h
index a250757..af0af90 100644
--- a/runtime/vm/compiler/assembler/assembler_arm64.h
+++ b/runtime/vm/compiler/assembler/assembler_arm64.h
@@ -1743,11 +1743,24 @@
 #endif  // defined(DART_COMPRESSED_POINTERS)
   }
 
-  void LoadWordFromBoxOrSmi(Register result, Register value) {
-    LoadInt64FromBoxOrSmi(result, value);

+  // Truncates upper bits.

+  void LoadInt32FromBoxOrSmi(Register result, Register value) override {

+    if (result == value) {
+      ASSERT(TMP != value);
+      MoveRegister(TMP, value);
+      value = TMP;
+    }
+    ASSERT(value != result);
+    compiler::Label done;
+    sbfx(result, value, kSmiTagSize,
+         Utils::Minimum(static_cast<int>(32), compiler::target::kSmiBits));
+    BranchIfSmi(value, &done);
+    LoadFieldFromOffset(result, value, compiler::target::Mint::value_offset(),
+                        compiler::kFourBytes);
+    Bind(&done);
   }
 

-  void LoadInt64FromBoxOrSmi(Register result, Register value) {
+  void LoadInt64FromBoxOrSmi(Register result, Register value) override {

     if (result == value) {
       ASSERT(TMP != value);

       MoveRegister(TMP, value);
diff --git a/runtime/vm/compiler/assembler/assembler_base.h b/runtime/vm/compiler/assembler/assembler_base.h
index 977fb9c..1da34fc 100644
--- a/runtime/vm/compiler/assembler/assembler_base.h
+++ b/runtime/vm/compiler/assembler/assembler_base.h
@@ -833,6 +833,22 @@
                             Register address,
                             int32_t offset = 0) = 0;

 
+  // Truncates upper bits.

+  virtual void LoadInt32FromBoxOrSmi(Register result, Register value) = 0;
+
+#if !defined(TARGET_ARCH_IS_32_BIT)
+  virtual void LoadInt64FromBoxOrSmi(Register result, Register value) = 0;
+#endif
+
+  // Truncates upper bits on 32 bit archs.
+  void LoadWordFromBoxOrSmi(Register result, Register value) {
+#if defined(TARGET_ARCH_IS_32_BIT)
+    LoadInt32FromBoxOrSmi(result, value);
+#else
+    LoadInt64FromBoxOrSmi(result, value);
+#endif
+  }
+
   // Loads nullability from an AbstractType [type] to [dst].
   void LoadAbstractTypeNullability(Register dst, Register type);
   // Loads nullability from an AbstractType [type] and compares it
diff --git a/runtime/vm/compiler/assembler/assembler_ia32.h b/runtime/vm/compiler/assembler/assembler_ia32.h
index 90289f2..e86af16 100644
--- a/runtime/vm/compiler/assembler/assembler_ia32.h
+++ b/runtime/vm/compiler/assembler/assembler_ia32.h
@@ -1075,11 +1075,8 @@
 
   void SmiUntag(Register reg) { sarl(reg, Immediate(kSmiTagSize)); }
 
-  void LoadWordFromBoxOrSmi(Register result, Register value) {
-    LoadInt32FromBoxOrSmi(result, value);
-  }
-
-  void LoadInt32FromBoxOrSmi(Register result, Register value) {

+  // Truncates upper bits.

+  void LoadInt32FromBoxOrSmi(Register result, Register value) override {

     if (result != value) {
       MoveRegister(result, value);

       value = result;
diff --git a/runtime/vm/compiler/assembler/assembler_riscv.h b/runtime/vm/compiler/assembler/assembler_riscv.h
index 15471a5..9122af1 100644
--- a/runtime/vm/compiler/assembler/assembler_riscv.h
+++ b/runtime/vm/compiler/assembler/assembler_riscv.h
@@ -958,15 +958,8 @@
   void SmiTag(Register reg) override { SmiTag(reg, reg); }
   void SmiTag(Register dst, Register src) { slli(dst, src, kSmiTagSize); }
 
-  void LoadWordFromBoxOrSmi(Register result, Register value) {
-#if XLEN == 32
-    LoadInt32FromBoxOrSmi(result, value);
-#else
-    LoadInt64FromBoxOrSmi(result, value);
-#endif
-  }
-
-  void LoadInt32FromBoxOrSmi(Register result, Register value) {

+  // Truncates upper bits.

+  void LoadInt32FromBoxOrSmi(Register result, Register value) override {

     if (result == value) {
       ASSERT(TMP != value);

       MoveRegister(TMP, value);
@@ -981,23 +974,21 @@
     Bind(&done);
   }
 
-  void LoadInt64FromBoxOrSmi(Register result, Register value) {
+#if XLEN != 32
+  void LoadInt64FromBoxOrSmi(Register result, Register value) override {

     if (result == value) {
       ASSERT(TMP != value);

       MoveRegister(TMP, value);
       value = TMP;
     }
-#if XLEN == 32
-    UNIMPLEMENTED();
-#else
     ASSERT(value != result);
     compiler::Label done;
     SmiUntag(result, value);
     BranchIfSmi(value, &done, compiler::Assembler::kNearJump);
     LoadFieldFromOffset(result, value, target::Mint::value_offset());
     Bind(&done);
-#endif
   }
+#endif
 
   void BranchIfNotSmi(Register reg,
                       Label* label,
diff --git a/runtime/vm/compiler/assembler/assembler_test.cc b/runtime/vm/compiler/assembler/assembler_test.cc
index 7b95f50..c4f7cba 100644

--- a/runtime/vm/compiler/assembler/assembler_test.cc
+++ b/runtime/vm/compiler/assembler/assembler_test.cc
@@ -147,19 +147,19 @@

 const Register kArg2Reg = CallingConventions::ArgumentRegisters[1];

 #endif
 
-#define LOAD_FROM_BOX_TEST(VALUE, SAME_REGISTER)                               \
-  ASSEMBLER_TEST_GENERATE(LoadWordFromBoxOrSmi##VALUE##SAME_REGISTER,          \
+#define LOAD_FROM_BOX_TEST(SIZE, TYPE, VALUE, SAME_REGISTER)                   \
+  ASSEMBLER_TEST_GENERATE(Load##SIZE##FromBoxOrSmi##VALUE##SAME_REGISTER,      \
                           assembler) {                                         \
     const bool same_register = SAME_REGISTER;                                  \
                                                                                \

     const Register src = kArg1Reg;                                             \
     const Register dst = same_register ? src : kArg2Reg;                       \

index 8d3fa8f..435cb5f 100644
--- a/runtime/vm/compiler/assembler/assembler_x64.h
+++ b/runtime/vm/compiler/assembler/assembler_x64.h
@@ -1072,11 +1072,10 @@
 #endif
   }
 
-  void LoadWordFromBoxOrSmi(Register result, Register value) {
-    LoadInt64FromBoxOrSmi(result, value);
-  }

+  // Truncates upper bits.

+  void LoadInt32FromBoxOrSmi(Register result, Register value) override;
 
-  void LoadInt64FromBoxOrSmi(Register result, Register value);
+  void LoadInt64FromBoxOrSmi(Register result, Register value) override;
 
   void BranchIfNotSmi(Register reg,
                       Label* label,

index fc3617d..d05c423 100644

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: merged

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 7

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

[CBuild (Gerrit)]

go/dart-cbuild result: SUCCESS

Details: https://goto.google.com/dart-cbuild/find/0e9e0002649364ed57c5fda8175bf484ddc6a3f4

View Change

To view, visit change 328521. To unsubscribe, or for help writing mail filters, visit settings.

Gerrit-MessageType: comment

Gerrit-Project: sdk

Gerrit-Branch: main

Gerrit-Change-Id: Ib697a0beae97968a0d1ae50c54b5769d9c2a23de

Gerrit-Change-Number: 328521

Gerrit-PatchSet: 7

Gerrit-Owner: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Commit Queue <dart-...@luci-project-accounts.iam.gserviceaccount.com>

Gerrit-Reviewer: Daco Harkes <dacoh...@google.com>

Gerrit-Reviewer: Tess Strickland <sstr...@google.com>

Gerrit-CC: CBuild <dart-inter...@dart-ci-internal.iam.gserviceaccount.com>

Gerrit-Comment-Date: Sat, 30 Sep 2023 08:49:23 +0000

Gerrit-HasComments: No

Gerrit-Has-Labels: No