From b275bde1afaf8de2be3e6decb42fe52dfa223d4d Mon Sep 17 00:00:00 2001 From: Michael Leon Date: Tue, 26 Nov 2024 21:22:46 -0800 Subject: [PATCH] Fix simplifyGetConstructedObjectInst Summary: We need to make sure that when replacing `GetConstructedObjectInst`, the value we are replacing it with is also an object type. It may not have been inferred yet. Reviewed By: avp Differential Revision: D66516220 fbshipit-source-id: 4046d0d0147fdbdb15a169b70ef4f4c5abb0078b --- lib/Optimizer/Scalar/InstSimplify.cpp | 8 ++- test/Optimizer/callee.js | 3 +- test/Optimizer/constructor_callee.js | 3 +- .../function-analysis-construction-store.js | 3 +- .../function-analysis-construction.js | 3 +- .../Optimizer/function-analysis-new-target.js | 3 +- test/Optimizer/inline-sibling-constructors.js | 3 +- test/Optimizer/regress-construct.js | 68 +++++++++++++++++++ 8 files changed, 87 insertions(+), 7 deletions(-) create mode 100644 test/Optimizer/regress-construct.js diff --git a/lib/Optimizer/Scalar/InstSimplify.cpp b/lib/Optimizer/Scalar/InstSimplify.cpp index cc2cb5f3c0e..b60a12e0e2a 100644 --- a/lib/Optimizer/Scalar/InstSimplify.cpp +++ b/lib/Optimizer/Scalar/InstSimplify.cpp @@ -655,7 +655,13 @@ class InstSimplifyImpl { auto opTy = GCOI->getConstructorReturnValue()->getType(); if (opTy.isObjectType()) return GCOI->getConstructorReturnValue(); - if (!opTy.canBeObject()) + // Ideally, all we would need to check is that opTy cannot be an object. + // However, there might be cases where we have inferred the return type of + // ConstructorReturnValue, and have not yet inferred that ThisValue is an + // object. In that case, it would be invalid to replace + // GetConstructedObject, which is of type object, with ThisValue, which is + // not of type object. + if (!opTy.canBeObject() && GCOI->getThisValue()->getType().isObjectType()) return GCOI->getThisValue(); return nullptr; } diff --git a/test/Optimizer/callee.js b/test/Optimizer/callee.js index c2e9de07b3b..4b7026ad472 100644 --- a/test/Optimizer/callee.js +++ b/test/Optimizer/callee.js @@ -82,7 +82,8 @@ function load_store_multiple_test() { // CHECK-NEXT: %1 = CreateFunctionInst (:object) %0: environment, %"foo 1#"(): functionCode // CHECK-NEXT: %2 = CreateThisInst (:undefined|object) %1: object, empty: any // CHECK-NEXT: %3 = CallInst (:number) %1: object, %"foo 1#"(): functionCode, true: boolean, empty: any, undefined: undefined, 0: number, 12: number -// CHECK-NEXT: ReturnInst %2: undefined|object +// CHECK-NEXT: %4 = GetConstructedObjectInst (:object) %2: undefined|object, 12: number +// CHECK-NEXT: ReturnInst %4: object // CHECK-NEXT:function_end // CHECK:scope %VS1 [k: object] diff --git a/test/Optimizer/constructor_callee.js b/test/Optimizer/constructor_callee.js index e6423403327..5f9fbe96d26 100644 --- a/test/Optimizer/constructor_callee.js +++ b/test/Optimizer/constructor_callee.js @@ -89,5 +89,6 @@ function ctor_load_store_test() { // CHECK-NEXT: %1 = LoadFrameInst (:object) %0: environment, [%VS1.use_this]: object // CHECK-NEXT: %2 = CreateThisInst (:undefined|object) %1: object, empty: any // CHECK-NEXT: %3 = CallInst (:undefined) %1: object, %"use_this 1#"(): functionCode, true: boolean, empty: any, undefined: undefined, %2: undefined|object, 12: number -// CHECK-NEXT: ReturnInst %2: undefined|object +// CHECK-NEXT: %4 = GetConstructedObjectInst (:object) %2: undefined|object, undefined: undefined +// CHECK-NEXT: ReturnInst %4: object // CHECK-NEXT:function_end diff --git a/test/Optimizer/function-analysis-construction-store.js b/test/Optimizer/function-analysis-construction-store.js index d56ec017267..a97eaa2f0a6 100644 --- a/test/Optimizer/function-analysis-construction-store.js +++ b/test/Optimizer/function-analysis-construction-store.js @@ -35,7 +35,8 @@ function main() { // CHECK-NEXT: %1 = CreateFunctionInst (:object) %0: environment, %f(): functionCode // CHECK-NEXT: %2 = CreateThisInst (:undefined|object) %1: object, empty: any // CHECK-NEXT: %3 = CallInst (:undefined) %1: object, %f(): functionCode, true: boolean, empty: any, undefined: undefined, 0: number -// CHECK-NEXT: ReturnInst %2: undefined|object +// CHECK-NEXT: %4 = GetConstructedObjectInst (:object) %2: undefined|object, undefined: undefined +// CHECK-NEXT: ReturnInst %4: object // CHECK-NEXT:function_end // CHECK:function f(): undefined [allCallsitesKnownInStrictMode] diff --git a/test/Optimizer/function-analysis-construction.js b/test/Optimizer/function-analysis-construction.js index c92dae09950..98c63ed9c5b 100644 --- a/test/Optimizer/function-analysis-construction.js +++ b/test/Optimizer/function-analysis-construction.js @@ -34,7 +34,8 @@ function main() { // CHECK-NEXT: %1 = CreateFunctionInst (:object) %0: environment, %f(): functionCode // CHECK-NEXT: %2 = CreateThisInst (:undefined|object) %1: object, empty: any // CHECK-NEXT: %3 = CallInst (:undefined) %1: object, %f(): functionCode, true: boolean, empty: any, undefined: undefined, 0: number -// CHECK-NEXT: ReturnInst %2: undefined|object +// CHECK-NEXT: %4 = GetConstructedObjectInst (:object) %2: undefined|object, undefined: undefined +// CHECK-NEXT: ReturnInst %4: object // CHECK-NEXT:function_end // CHECK:function f(): undefined [allCallsitesKnownInStrictMode] diff --git a/test/Optimizer/function-analysis-new-target.js b/test/Optimizer/function-analysis-new-target.js index bd701672050..b085954656a 100644 --- a/test/Optimizer/function-analysis-new-target.js +++ b/test/Optimizer/function-analysis-new-target.js @@ -37,7 +37,8 @@ function main() { // CHECK-NEXT: %1 = CreateFunctionInst (:object) %0: environment, %f(): functionCode // CHECK-NEXT: %2 = CreateThisInst (:undefined|object) %1: object, empty: any // CHECK-NEXT: %3 = CallInst (:undefined) %1: object, %f(): functionCode, true: boolean, empty: any, %1: object, %2: undefined|object -// CHECK-NEXT: ReturnInst %2: undefined|object +// CHECK-NEXT: %4 = GetConstructedObjectInst (:object) %2: undefined|object, undefined: undefined +// CHECK-NEXT: ReturnInst %4: object // CHECK-NEXT:function_end // CHECK:function f(): undefined diff --git a/test/Optimizer/inline-sibling-constructors.js b/test/Optimizer/inline-sibling-constructors.js index 2c23478e672..a651fbcc915 100644 --- a/test/Optimizer/inline-sibling-constructors.js +++ b/test/Optimizer/inline-sibling-constructors.js @@ -61,5 +61,6 @@ function outer() { // CHECK-NEXT: StorePropertyStrictInst %1: any, %5: undefined|object, "x": string // CHECK-NEXT: StorePropertyStrictInst %2: any, %5: undefined|object, "y": string // CHECK-NEXT: StorePropertyStrictInst %3: any, %5: undefined|object, "z": string -// CHECK-NEXT: ReturnInst %5: undefined|object +// CHECK-NEXT: %9 = GetConstructedObjectInst (:object) %5: undefined|object, undefined: undefined +// CHECK-NEXT: ReturnInst %9: object // CHECK-NEXT:function_end diff --git a/test/Optimizer/regress-construct.js b/test/Optimizer/regress-construct.js new file mode 100644 index 00000000000..baf032943c9 --- /dev/null +++ b/test/Optimizer/regress-construct.js @@ -0,0 +1,68 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// RUN: %hermesc %s -dump-ir | %FileCheckOrRegen --match-full-lines %s + +// Check that we don't replace GetConstructedObjectInst with a +// value which does not also have an object type. +// +// IRVerifier error: StoreFrameInst: Value type mismatch in function + +function main() { + var x = null; + function y() {} + return function value() { + return x || (x = new y()); + }; +} + +// Auto-generated content below. Please do not modify manually. + +// CHECK:scope %VS0 [] + +// CHECK:function global(): undefined +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = CreateScopeInst (:environment) %VS0: any, empty: any +// CHECK-NEXT: DeclareGlobalVarInst "main": string +// CHECK-NEXT: %2 = CreateFunctionInst (:object) %0: environment, %main(): functionCode +// CHECK-NEXT: StorePropertyLooseInst %2: object, globalObject: object, "main": string +// CHECK-NEXT: ReturnInst undefined: undefined +// CHECK-NEXT:function_end + +// CHECK:scope %VS1 [x: null|object, y: object] + +// CHECK:function main(): object +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = GetParentScopeInst (:environment) %VS0: any, %parentScope: environment +// CHECK-NEXT: %1 = CreateScopeInst (:environment) %VS1: any, %0: environment +// CHECK-NEXT: %2 = CreateFunctionInst (:object) %1: environment, %y(): functionCode +// CHECK-NEXT: StoreFrameInst %1: environment, %2: object, [%VS1.y]: object +// CHECK-NEXT: StoreFrameInst %1: environment, null: null, [%VS1.x]: null|object +// CHECK-NEXT: %5 = CreateFunctionInst (:object) %1: environment, %value(): functionCode +// CHECK-NEXT: ReturnInst %5: object +// CHECK-NEXT:function_end + +// CHECK:function y(): undefined [allCallsitesKnownInStrictMode,unreachable] +// CHECK-NEXT:%BB0: +// CHECK-NEXT: UnreachableInst +// CHECK-NEXT:function_end + +// CHECK:function value(): null|object +// CHECK-NEXT:%BB0: +// CHECK-NEXT: %0 = GetParentScopeInst (:environment) %VS1: any, %parentScope: environment +// CHECK-NEXT: %1 = LoadFrameInst (:null|object) %0: environment, [%VS1.x]: null|object +// CHECK-NEXT: CondBranchInst %1: null|object, %BB2, %BB1 +// CHECK-NEXT:%BB1: +// CHECK-NEXT: %3 = LoadFrameInst (:object) %0: environment, [%VS1.y]: object +// CHECK-NEXT: %4 = CreateThisInst (:undefined|object) %3: object, empty: any +// CHECK-NEXT: %5 = GetConstructedObjectInst (:object) %4: undefined|object, undefined: undefined +// CHECK-NEXT: StoreFrameInst %0: environment, %5: object, [%VS1.x]: null|object +// CHECK-NEXT: BranchInst %BB2 +// CHECK-NEXT:%BB2: +// CHECK-NEXT: %8 = PhiInst (:null|object) %1: null|object, %BB0, %5: object, %BB1 +// CHECK-NEXT: ReturnInst %8: null|object +// CHECK-NEXT:function_end