[codegen][x64] Add an in place version of Smi(Un)Tag

The current version of SmiTag and SmiUntag was checking if the
registers were the same, copying them if not and then untagging.
We can avoid a branch and a check by having two versions of
SmiTag and SmiUntag.

Change-Id: Id89213e073cefc9f8e46fcf0e79d0c1d349342ae
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1826730Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Cr-Commit-Position: refs/heads/master@{#64021}

src/builtins/x64/builtins-x64.cc

@@ -1201,8 +1201,7 @@ void Builtins::Generate_InterpreterEntryTrampoline(MacroAssembler* masm) {
           Operand(rbp, InterpreterFrameConstants::kBytecodeArrayFromFp));
   __ movq(kInterpreterBytecodeOffsetRegister,
           Operand(rbp, InterpreterFrameConstants::kBytecodeOffsetFromFp));
-  __ SmiUntag(kInterpreterBytecodeOffsetRegister,
-              kInterpreterBytecodeOffsetRegister);
+  __ SmiUntag(kInterpreterBytecodeOffsetRegister);
 
   // Either return, or advance to the next bytecode and dispatch.
   Label do_return;
@@ -1429,8 +1428,7 @@ static void Generate_InterpreterEnterBytecode(MacroAssembler* masm) {
   // Get the target bytecode offset from the frame.
   __ movq(kInterpreterBytecodeOffsetRegister,
           Operand(rbp, InterpreterFrameConstants::kBytecodeOffsetFromFp));
-  __ SmiUntag(kInterpreterBytecodeOffsetRegister,
-              kInterpreterBytecodeOffsetRegister);
+  __ SmiUntag(kInterpreterBytecodeOffsetRegister);
 
   // Dispatch to the target bytecode.
   __ movzxbq(r11, Operand(kInterpreterBytecodeArrayRegister,
@@ -1447,8 +1445,7 @@ void Builtins::Generate_InterpreterEnterBytecodeAdvance(MacroAssembler* masm) {
           Operand(rbp, InterpreterFrameConstants::kBytecodeArrayFromFp));
   __ movq(kInterpreterBytecodeOffsetRegister,
           Operand(rbp, InterpreterFrameConstants::kBytecodeOffsetFromFp));
-  __ SmiUntag(kInterpreterBytecodeOffsetRegister,
-              kInterpreterBytecodeOffsetRegister);
+  __ SmiUntag(kInterpreterBytecodeOffsetRegister);
 
   // Load the current bytecode.
   __ movzxbq(rbx, Operand(kInterpreterBytecodeArrayRegister,
@@ -1487,7 +1484,7 @@ void Builtins::Generate_InstantiateAsmJs(MacroAssembler* masm) {
     // Preserve argument count for later compare.
     __ movq(rcx, rax);
     // Push the number of arguments to the callee.
-    __ SmiTag(rax, rax);
+    __ SmiTag(rax);
     __ Push(rax);
     // Push a copy of the target function and the new target.
     __ Push(rdi);
@@ -1524,7 +1521,7 @@ void Builtins::Generate_InstantiateAsmJs(MacroAssembler* masm) {
 
     __ Drop(2);
     __ Pop(rcx);
-    __ SmiUntag(rcx, rcx);
+    __ SmiUntag(rcx);
     scope.GenerateLeaveFrame();
 
     __ PopReturnAddressTo(rbx);
@@ -1538,7 +1535,7 @@ void Builtins::Generate_InstantiateAsmJs(MacroAssembler* masm) {
     __ Pop(rdx);
     __ Pop(rdi);
     __ Pop(rax);
-    __ SmiUntag(rax, rax);
+    __ SmiUntag(rax);
   }
   // On failure, tail call back to regular js by re-calling the function
   // which has be reset to the compile lazy builtin.
@@ -1565,7 +1562,7 @@ void Generate_ContinueToBuiltinHelper(MacroAssembler* masm,
     int code = config->GetAllocatableGeneralCode(i);
     __ popq(Register::from_code(code));
     if (java_script_builtin && code == kJavaScriptCallArgCountRegister.code()) {
-      __ SmiUntag(Register::from_code(code), Register::from_code(code));
+      __ SmiUntag(Register::from_code(code));
     }
   }
   __ movq(
@@ -2276,7 +2273,7 @@ void Builtins::Generate_CallFunction(MacroAssembler* masm,
         // TODO(bmeurer): Inline the allocation here to avoid building the frame
         // in the fast case? (fall back to AllocateInNewSpace?)
         FrameScope scope(masm, StackFrame::INTERNAL);
-        __ SmiTag(rax, rax);
+        __ SmiTag(rax);
         __ Push(rax);
         __ Push(rdi);
         __ movq(rax, rcx);
@@ -2287,7 +2284,7 @@ void Builtins::Generate_CallFunction(MacroAssembler* masm,
         __ movq(rcx, rax);
         __ Pop(rdi);
         __ Pop(rax);
-        __ SmiUntag(rax, rax);
+        __ SmiUntag(rax);
       }
       __ LoadTaggedPointerField(
           rdx, FieldOperand(rdi, JSFunction::kSharedFunctionInfoOffset));
@@ -2649,7 +2646,7 @@ void Builtins::Generate_WasmCompileLazy(MacroAssembler* masm) {
   // The function index was pushed to the stack by the caller as int32.
   __ Pop(r11);
   // Convert to Smi for the runtime call.
-  __ SmiTag(r11, r11);
+  __ SmiTag(r11);
   {
     HardAbortScope hard_abort(masm);  // Avoid calls to Abort.
     FrameScope scope(masm, StackFrame::WASM_COMPILE_LAZY);

src/codegen/x64/macro-assembler-x64.cc

@@ -1133,34 +1133,49 @@ void TurboAssembler::Move(Register dst, ExternalReference ext) {
   movq(dst, Immediate64(ext.address(), RelocInfo::EXTERNAL_REFERENCE));
 }
 
-void MacroAssembler::SmiTag(Register dst, Register src) {
+void MacroAssembler::SmiTag(Register reg) {
   STATIC_ASSERT(kSmiTag == 0);
   DCHECK(SmiValuesAre32Bits() || SmiValuesAre31Bits());
   if (COMPRESS_POINTERS_BOOL) {
-    if (dst != src) {
-      movl(dst, src);
-    }
-    shll(dst, Immediate(kSmiShift));
+    shll(reg, Immediate(kSmiShift));
   } else {
-    if (dst != src) {
-      movq(dst, src);
-    }
-    shlq(dst, Immediate(kSmiShift));
+    shlq(reg, Immediate(kSmiShift));
   }
 }
 
-void TurboAssembler::SmiUntag(Register dst, Register src) {
+void MacroAssembler::SmiTag(Register dst, Register src) {
+  DCHECK(dst != src);
+  if (COMPRESS_POINTERS_BOOL) {
+    movl(dst, src);
+  } else {
+    movq(dst, src);
+  }
+  SmiTag(dst);
+}
+
+void TurboAssembler::SmiUntag(Register reg) {
   STATIC_ASSERT(kSmiTag == 0);
   DCHECK(SmiValuesAre32Bits() || SmiValuesAre31Bits());
+  // TODO(v8:7703): Is there a way to avoid this sign extension when pointer
+  // compression is enabled?
+  if (COMPRESS_POINTERS_BOOL) {
+    movsxlq(reg, reg);
+  }
+  sarq(reg, Immediate(kSmiShift));
+}
+
+void TurboAssembler::SmiUntag(Register dst, Register src) {
+  DCHECK(dst != src);
   if (COMPRESS_POINTERS_BOOL) {
     movsxlq(dst, src);
-    sarq(dst, Immediate(kSmiShift));
   } else {
-    if (dst != src) {
-      movq(dst, src);
-    }
-    sarq(dst, Immediate(kSmiShift));
+    movq(dst, src);
   }
+  // TODO(v8:7703): Call SmiUntag(reg) if we can find a way to avoid the extra
+  // mov when pointer compression is enabled.
+  STATIC_ASSERT(kSmiTag == 0);
+  DCHECK(SmiValuesAre32Bits() || SmiValuesAre31Bits());
+  sarq(dst, Immediate(kSmiShift));
 }
 
 void TurboAssembler::SmiUntag(Register dst, Operand src) {
@@ -1617,7 +1632,7 @@ void TurboAssembler::Call(Handle<Code> code_object, RelocInfo::Mode rmode) {
 Operand TurboAssembler::EntryFromBuiltinIndexAsOperand(Register builtin_index) {
   if (SmiValuesAre32Bits()) {
     // The builtin_index register contains the builtin index as a Smi.
-    SmiUntag(builtin_index, builtin_index);
+    SmiUntag(builtin_index);
     return Operand(kRootRegister, builtin_index, times_system_pointer_size,
                    IsolateData::builtin_entry_table_offset());
   } else {
@@ -2387,13 +2402,13 @@ void MacroAssembler::CheckDebugHook(Register fun, Register new_target,
     FrameScope frame(this,
                      has_frame() ? StackFrame::NONE : StackFrame::INTERNAL);
     if (expected.is_reg()) {
-      SmiTag(expected.reg(), expected.reg());
+      SmiTag(expected.reg());
       Push(expected.reg());
     }
     if (actual.is_reg()) {
-      SmiTag(actual.reg(), actual.reg());
+      SmiTag(actual.reg());
       Push(actual.reg());
-      SmiUntag(actual.reg(), actual.reg());
+      SmiUntag(actual.reg());
     }
     if (new_target.is_valid()) {
       Push(new_target);
@@ -2408,11 +2423,11 @@ void MacroAssembler::CheckDebugHook(Register fun, Register new_target,
     }
     if (actual.is_reg()) {
       Pop(actual.reg());
-      SmiUntag(actual.reg(), actual.reg());
+      SmiUntag(actual.reg());
     }
     if (expected.is_reg()) {
       Pop(expected.reg());
-      SmiUntag(expected.reg(), expected.reg());
+      SmiUntag(expected.reg());
     }
   }
   bind(&skip_hook);

src/codegen/x64/macro-assembler-x64.h

@@ -314,6 +314,8 @@ class V8_EXPORT_PRIVATE TurboAssembler : public TurboAssemblerBase {
       RelocInfo::Mode rmode = RelocInfo::FULL_EMBEDDED_OBJECT);
 
   // Convert smi to word-size sign-extended value.
+  void SmiUntag(Register reg);
+  // Requires dst != src
   void SmiUntag(Register dst, Register src);
   void SmiUntag(Register dst, Operand src);
 
@@ -665,6 +667,8 @@ class V8_EXPORT_PRIVATE MacroAssembler : public TurboAssembler {
   // Conversions between tagged smi values and non-tagged integer values.
 
   // Tag an word-size value. The result must be known to be a valid smi value.
+  void SmiTag(Register reg);
+  // Requires dst != src
   void SmiTag(Register dst, Register src);
 
   // Simple comparison of smis.  Both sides must be known smis to use these,

test/cctest/test-macro-assembler-x64.cc

@@ -242,35 +242,35 @@ TEST(SmiTag) {
 
   __ movq(rax, Immediate(1));  // Test number.
   __ movq(rcx, Immediate(0));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   __ Set(rdx, Smi::kZero.ptr());
   __ cmp_tagged(rcx, rdx);
   __ j(not_equal, &exit);
 
   __ movq(rax, Immediate(2));  // Test number.
   __ movq(rcx, Immediate(1024));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   __ Set(rdx, Smi::FromInt(1024).ptr());
   __ cmp_tagged(rcx, rdx);
   __ j(not_equal, &exit);
 
   __ movq(rax, Immediate(3));  // Test number.
   __ movq(rcx, Immediate(-1));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   __ Set(rdx, Smi::FromInt(-1).ptr());
   __ cmp_tagged(rcx, rdx);
   __ j(not_equal, &exit);
 
   __ movq(rax, Immediate(4));  // Test number.
   __ movq(rcx, Immediate(Smi::kMaxValue));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   __ Set(rdx, Smi::FromInt(Smi::kMaxValue).ptr());
   __ cmp_tagged(rcx, rdx);
   __ j(not_equal, &exit);
 
   __ movq(rax, Immediate(5));  // Test number.
   __ movq(rcx, Immediate(Smi::kMinValue));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   __ Set(rdx, Smi::FromInt(Smi::kMinValue).ptr());
   __ cmp_tagged(rcx, rdx);
   __ j(not_equal, &exit);
@@ -344,7 +344,7 @@ TEST(SmiCheck) {
   // CheckSmi
 
   __ movl(rcx, Immediate(0));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   cond = masm->CheckSmi(rcx);
   __ j(NegateCondition(cond), &exit);
 
@@ -355,7 +355,7 @@ TEST(SmiCheck) {
 
   __ incq(rax);
   __ movl(rcx, Immediate(-1));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   cond = masm->CheckSmi(rcx);
   __ j(NegateCondition(cond), &exit);
 
@@ -366,7 +366,7 @@ TEST(SmiCheck) {
 
   __ incq(rax);
   __ movl(rcx, Immediate(Smi::kMaxValue));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   cond = masm->CheckSmi(rcx);
   __ j(NegateCondition(cond), &exit);
 
@@ -377,7 +377,7 @@ TEST(SmiCheck) {
 
   __ incq(rax);
   __ movl(rcx, Immediate(Smi::kMinValue));
-  __ SmiTag(rcx, rcx);
+  __ SmiTag(rcx);
   cond = masm->CheckSmi(rcx);
   __ j(NegateCondition(cond), &exit);