From 7bd8b3130a90326b78ee6ff3731688953a4231e6 Mon Sep 17 00:00:00 2001 From: Alexander Markov Date: Tue, 18 Jun 2024 18:53:18 +0000 Subject: [PATCH] [vm/compiler] Make flow graph construction independent of --target_unknown_cpu Move optimized handling of Double.truncateToDouble, floorToDouble, ceilToDouble, floor and ceil from flow graph construction to the canonicalization pass. This is needed in order to maintain stable flow graphs between app-jit snapshot training (which is performed with --target_unknown_cpu) and running from app-jit snapshot without --target_unknown_cpu. TEST=ci (vm-appjit-*) Fixes https://github.com/dart-lang/sdk/issues/56018 Change-Id: I7e3e4ce917984f52db239506ea3de0c55af00567 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/372140 Reviewed-by: Slava Egorov Commit-Queue: Alexander Markov --- runtime/bin/main_impl.cc | 7 --- runtime/vm/compiler/backend/il.cc | 61 ++++++++++++++++++++ runtime/vm/compiler/backend/il.h | 8 ++- runtime/vm/compiler/backend/il_arm.cc | 1 + runtime/vm/compiler/frontend/kernel_to_il.cc | 36 +----------- 5 files changed, 71 insertions(+), 42 deletions(-) diff --git a/runtime/bin/main_impl.cc b/runtime/bin/main_impl.cc index 025317836e17..934b351944cb 100644 --- a/runtime/bin/main_impl.cc +++ b/runtime/bin/main_impl.cc @@ -1321,13 +1321,6 @@ void main(int argc, char** argv) { vm_options.AddArgument("--link_natives_lazily"); #endif } - if (app_snapshot != nullptr && app_snapshot->IsJIT()) { - // App-jit snapshot was created with this flag, it also has to be added - // when running the snapshot when it is depended upon by the graph builder - // (see e.g. FlowGraphBuilder::IsRecognizedMethodForFlowGraph). - // TODO(dartbug.com/56018) Better fix leaving less performance on the table. - vm_options.AddArgument("--target-unknown-cpu"); - } // If we need to write an app-jit snapshot or a depfile, then add an exit // hook that writes the snapshot and/or depfile as appropriate. diff --git a/runtime/vm/compiler/backend/il.cc b/runtime/vm/compiler/backend/il.cc index bf707da3df92..a1627c279d08 100644 --- a/runtime/vm/compiler/backend/il.cc +++ b/runtime/vm/compiler/backend/il.cc @@ -7260,6 +7260,67 @@ const RuntimeEntry& InvokeMathCFunctionInstr::TargetFunction() const { return kLibcPowRuntimeEntry; } +Definition* InvokeMathCFunctionInstr::Canonicalize(FlowGraph* flow_graph) { + if (!CompilerState::Current().is_aot() && + TargetCPUFeatures::double_truncate_round_supported()) { + Token::Kind op_kind = Token::kILLEGAL; + switch (recognized_kind_) { + case MethodRecognizer::kDoubleTruncateToDouble: + op_kind = Token::kTRUNCATE; + break; + case MethodRecognizer::kDoubleFloorToDouble: + op_kind = Token::kFLOOR; + break; + case MethodRecognizer::kDoubleCeilToDouble: + op_kind = Token::kCEILING; + break; + default: + return this; + } + auto* instr = new UnaryDoubleOpInstr( + op_kind, new Value(InputAt(0)->definition()), GetDeoptId(), + Instruction::kNotSpeculative, kUnboxedDouble); + flow_graph->InsertBefore(this, instr, env(), FlowGraph::kValue); + return instr; + } + + return this; +} + +bool DoubleToIntegerInstr::SupportsFloorAndCeil() { +#if defined(TARGET_ARCH_X64) + return CompilerState::Current().is_aot() || FLAG_target_unknown_cpu; +#elif defined(TARGET_ARCH_ARM64) || defined(TARGET_ARCH_RISCV32) || \ + defined(TARGET_ARCH_RISCV64) + return true; +#else + return false; +#endif +} + +Definition* DoubleToIntegerInstr::Canonicalize(FlowGraph* flow_graph) { + if (SupportsFloorAndCeil() && + (recognized_kind() == MethodRecognizer::kDoubleToInteger)) { + if (auto* arg = value()->definition()->AsInvokeMathCFunction()) { + switch (arg->recognized_kind()) { + case MethodRecognizer::kDoubleFloorToDouble: + // x.floorToDouble().toInt() => x.floor() + recognized_kind_ = MethodRecognizer::kDoubleFloorToInt; + value()->BindTo(arg->InputAt(0)->definition()); + break; + case MethodRecognizer::kDoubleCeilToDouble: + // x.ceilToDouble().toInt() => x.ceil() + recognized_kind_ = MethodRecognizer::kDoubleCeilToInt; + value()->BindTo(arg->InputAt(0)->definition()); + break; + default: + break; + } + } + } + return this; +} + TruncDivModInstr::TruncDivModInstr(Value* lhs, Value* rhs, intptr_t deopt_id) : TemplateDefinition(deopt_id) { SetInputAt(0, lhs); diff --git a/runtime/vm/compiler/backend/il.h b/runtime/vm/compiler/backend/il.h index 8d569f7020e8..076d2b5a2a50 100644 --- a/runtime/vm/compiler/backend/il.h +++ b/runtime/vm/compiler/backend/il.h @@ -10084,7 +10084,9 @@ class DoubleToIntegerInstr : public TemplateDefinition<1, Throws, Pure> { return other.AsDoubleToInteger()->recognized_kind() == recognized_kind(); } -#define FIELD_LIST(F) F(const MethodRecognizer::Kind, recognized_kind_) + virtual Definition* Canonicalize(FlowGraph* flow_graph); + +#define FIELD_LIST(F) F(MethodRecognizer::Kind, recognized_kind_) DECLARE_INSTRUCTION_SERIALIZABLE_FIELDS(DoubleToIntegerInstr, TemplateDefinition, @@ -10092,6 +10094,8 @@ class DoubleToIntegerInstr : public TemplateDefinition<1, Throws, Pure> { #undef FIELD_LIST private: + static bool SupportsFloorAndCeil(); + DISALLOW_COPY_AND_ASSIGN(DoubleToIntegerInstr); }; @@ -10284,6 +10288,8 @@ class InvokeMathCFunctionInstr : public VariadicDefinition { return other_invoke->recognized_kind() == recognized_kind(); } + virtual Definition* Canonicalize(FlowGraph* flow_graph); + virtual bool MayThrow() const { return false; } static constexpr intptr_t kSavedSpTempIndex = 0; diff --git a/runtime/vm/compiler/backend/il_arm.cc b/runtime/vm/compiler/backend/il_arm.cc index 4f8a3a020428..02d7e7e45c92 100644 --- a/runtime/vm/compiler/backend/il_arm.cc +++ b/runtime/vm/compiler/backend/il_arm.cc @@ -5578,6 +5578,7 @@ LocationSummary* DoubleToIntegerInstr::MakeLocationSummary(Zone* zone, } void DoubleToIntegerInstr::EmitNativeCode(FlowGraphCompiler* compiler) { + ASSERT(recognized_kind() == MethodRecognizer::kDoubleToInteger); const Register result = locs()->out(0).reg(); const DRegister value_double = EvenDRegisterOf(locs()->in(0).fpu_reg()); diff --git a/runtime/vm/compiler/frontend/kernel_to_il.cc b/runtime/vm/compiler/frontend/kernel_to_il.cc index 5d146140e806..c77afdfce4db 100644 --- a/runtime/vm/compiler/frontend/kernel_to_il.cc +++ b/runtime/vm/compiler/frontend/kernel_to_il.cc @@ -1140,16 +1140,6 @@ bool FlowGraphBuilder::IsRecognizedMethodForFlowGraph( case MethodRecognizer::kMathLog: case MethodRecognizer::kMathSqrt: return true; - case MethodRecognizer::kDoubleCeilToInt: - case MethodRecognizer::kDoubleFloorToInt: -#if defined(TARGET_ARCH_X64) - return CompilerState::Current().is_aot() || FLAG_target_unknown_cpu; -#elif defined(TARGET_ARCH_ARM64) || defined(TARGET_ARCH_RISCV32) || \ - defined(TARGET_ARCH_RISCV64) - return true; -#else - return false; -#endif default: return false; } @@ -1814,9 +1804,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod( body += LoadIndexed(kIntPtrCid); body += Box(kUnboxedIntPtr); } break; - case MethodRecognizer::kDoubleToInteger: - case MethodRecognizer::kDoubleCeilToInt: - case MethodRecognizer::kDoubleFloorToInt: { + case MethodRecognizer::kDoubleToInteger: { body += LoadLocal(parsed_function_->RawParameterVariable(0)); body += DoubleToInteger(kind); } break; @@ -1839,27 +1827,7 @@ FlowGraph* FlowGraphBuilder::BuildGraphOfRecognizedMethod( for (intptr_t i = 0, n = function.NumParameters(); i < n; ++i) { body += LoadLocal(parsed_function_->RawParameterVariable(i)); } - if (!CompilerState::Current().is_aot() && - TargetCPUFeatures::double_truncate_round_supported() && - ((kind == MethodRecognizer::kDoubleTruncateToDouble) || - (kind == MethodRecognizer::kDoubleFloorToDouble) || - (kind == MethodRecognizer::kDoubleCeilToDouble))) { - switch (kind) { - case MethodRecognizer::kDoubleTruncateToDouble: - body += UnaryDoubleOp(Token::kTRUNCATE); - break; - case MethodRecognizer::kDoubleFloorToDouble: - body += UnaryDoubleOp(Token::kFLOOR); - break; - case MethodRecognizer::kDoubleCeilToDouble: - body += UnaryDoubleOp(Token::kCEILING); - break; - default: - UNREACHABLE(); - } - } else { - body += InvokeMathCFunction(kind, function.NumParameters()); - } + body += InvokeMathCFunction(kind, function.NumParameters()); } break; case MethodRecognizer::kMathSqrt: { body += LoadLocal(parsed_function_->RawParameterVariable(0));