Tidy-Up IREval Logic So We're Not Edge-Casing As Much

[sebholl]

Summary

As part of my canonicalization work, I reviewed the logic in IREval that performs the constant expression evaluations.

I noticed a large amount of "edge-casing" in its implementation which was making it hard to spot gaps in its coverage.

• We supported the negate unary operator but not the plus unary operator, even though their implemented logic is mostly symmetric. This is now supported.
• Almost all of the numerical operators would bail if the operands were non-string primitives (e.g. false | null). These are now coerced to numbers and supported.
• The string concatenation operator would bail with non-string primitives. These are coerced to string and supported.
• We were edge-casing NaNs un-necessarily when the C++ compiler IEEE implementation will take care of most of the heavy lifting.

Following refactor, we can evaluate many new circumstances, and have reduced lines of code of IREval.cpp from 705 to 623 lines of code.

Follow-Up

In the future, it might be a better idea to get the compiler to just execute these operations on the VM and then simply map back from the VM output back to an IR literal. There's clearly maintenance risk in having a separate operation evaluator to the VM runtime itself.

Test Plan

I have added extra test cases to simplify.js and then made sure it runs:

cmake --build ../build --target check-hermes all -j 4
[29/30] Running the Hermes regression tests
Testing Time: 20.26s
  Expected Passes    : 1681
  Unsupported Tests  : 62

Using the following bootstrap, we also -exec the simplify.js test with v8 and also with Hermes to make sure the output is equivalent:

[add_sub_num, modulo_num, logic_ops_test, add_str, add_empty_str, add_empty_str_simplify, add_null, mul_null, left_shift_num, left_shift_null, left_shift_undefined, right_shift_num, right_shift_null, right_shift_undefined, right_shift_bool, unsigned_right_shift_bool, unsigned_right_shift_compound_assgmt, unsigned_right_shift_num, add_undef, comp_num, equality, arith, undef_test, foo, strip_bang, turn_unary_plus_into_as_number, turn_unary_plus_on_literal_into_result, turn_bitor_into_as_int32, unary_ops, test_phi, if_inline, simplify_switch, objectCond].forEach(function(test){
  test(1, 2, 3, 4, 5, 6);
});

In a 33MB real-world React Native bundle, we saw efficiencies manifest in string concatenation for some react-devtools code:

diff --git a/./bk-none.lir.normalized.txt b/./bk-ireval-everything.lir.normalized.txt
index ac87cc5..274c480 100644
--- a/./bk-none.lir.normalized.txt
+++ b/./bk-ireval-everything.lir.normalized.txt
@@ -157596,21 +157596,7 @@ S{" #"#()#} = [debug# : undefined|closure, MAX_LENGTH# : undefined|number, MAX_S
   %# = HBCCallNInst %# : closure, undefined : undefined, %# : undefined, %# : string
   %# = HBCLoadFromEnvironmentInst %#, [t#] : undefined|object
   %# = LoadPropertyInst %#, "COERCE" : string
-  %# = HBCLoadConstInst "(^|[^\\\\d])(\\\\d{#," : string
-  %# = HBCLoadConstInst # : number
-  %# = BinaryOperatorInst '+', %# : string, %# : number
-  %# = HBCLoadConstInst "})" : string
-  %# = BinaryOperatorInst '+', %# : string, %# : string
-  %# = HBCLoadConstInst "(?:\\\\.(\\\\d{#," : string
-  %# = BinaryOperatorInst '+', %# : string, %# : string
-  %# = BinaryOperatorInst '+', %# : string, %# : number
-  %# = HBCLoadConstInst "}))?" : string
-  %# = BinaryOperatorInst '+', %# : string, %# : string
-  %# = BinaryOperatorInst '+', %# : string, %# : string
-  %# = BinaryOperatorInst '+', %# : string, %# : number
-  %# = BinaryOperatorInst '+', %# : string, %# : string
-  %# = HBCLoadConstInst "(?:$|[^\\\\d])" : string
-  %# = BinaryOperatorInst '+', %# : string, %# : string
+  %# = HBCLoadConstInst "(^|[^\\\\d])(\\\\d{#,#})(?:\\\\.(\\\\d{#,#}))?(?:\\\\.(\\\\d{#,#}))?(?:$|[^\\\\d])" : string
   %# = StorePropertyInst %# : string, %# : object, %#
   %# = HBCLoadConstInst "COERCERTL" : string
   %# = HBCCallNInst %# : closure, undefined : undefined, %# : undefined, %# : string
@@ -423219,10 +423205,7 @@ S{" #"#()#} = []
   %# = LoadPropertyInst %#, "substring" : string
   %# = HBCLoadConstInst # : number
   %# = HBCCallNInst %#, undefined : undefined, %#, %# : number, %# : number
-  %# = HBCLoadConstInst "...(truncated to the first " : string
-  %# = BinaryOperatorInst '+', %# : string, %# : number
-  %# = HBCLoadConstInst " characters)" : string
-  %# = BinaryOperatorInst '+', %# : string, %# : string
+  %# = HBCLoadConstInst "...(truncated to the first # characters)" : string
   %# = BinaryOperatorInst '+', %#, %# : string
   %# = ReturnInst %# : string
 %BB#:
@@ -2211029,23 +2211012,13 @@ S{" #"#()#} = [_components#, _reactIntl#, _jsxRuntime#, TicketContainer#, Ticket
   %# = TryLoadGlobalPropertyInst %# : object, "Styles" : string
   %# = LoadPropertyInst %#, "zIndex" : string
   %# = LoadPropertyInst %#, "below" : string
-  %# = AllocObjectInst # : number, empty
-  %# = StoreNewOwnPropertyInst %#, %# : object, "zIndex" : string, true : boolean
-  %# = HBCLoadConstInst #.# : number
-  %# = HBCLoadConstInst "px" : string
-  %# = BinaryOperatorInst '+', %# : number, %# : string
-  %# = StoreNewOwnPropertyInst %# : string, %# : object, "paddingX" : string, true : boolean
-  %# = StoreNewOwnPropertyInst %# : number, %# : object, "paddingY" : string, true : boolean
+  %# = HBCAllocObjectFromBufferInst # : number, "zIndex" : string, null : null, "paddingX" : string, "#.#px" : string, "paddingY" : string, # : number
+  %# = StorePropertyInst %#, %# : object, "zIndex" : string
   %# = HBCCallNInst %#, undefined : undefined, %#, %# : object
   %# = HBCStoreToEnvironmentInst %#, %#, [OfferContent#]
   %# = LoadPropertyInst %#, "Box" : string
   %# = LoadPropertyInst %#, "withConfig" : string
-  %# = AllocObjectInst # : number, empty
-  %# = HBCLoadConstInst # : number
-  %# = BinaryOperatorInst '+', %# : number, %# : string
-  %# = StoreNewOwnPropertyInst %# : string, %# : object, "minHeight" : string, true : boolean
-  %# = HBCLoadConstInst "$#" : string
-  %# = StoreNewOwnPropertyInst %# : string, %# : object, "marginBottom" : string, true : boolean
+  %# = HBCAllocObjectFromBufferInst # : number, "minHeight" : string, "#px" : string, "marginBottom" : string, "$#" : string
   %# = HBCCallNInst %#, undefined : undefined, %#, %# : object
   %# = HBCStoreToEnvironmentInst %#, %#, [OfferContainer#]
   %# = LoadPropertyInst %#, "Box" : string
@@ -2211146,9 +2211119,6 @@ S{" #"#()#} = []
   %# = HBCLoadConstInst "auto" : string
   %# = HBCLoadConstInst # : number
   %# = HBCLoadConstInst -# : number
-  %# = HBCLoadConstInst # : number
-  %# = HBCLoadConstInst "px" : string
-  %# = BinaryOperatorInst '+', %# : number, %# : string
   %# = LoadPropertyInst %#, "bgColor" : string
   %# = LoadPropertyInst %#, "borderColor" : string
   %# = LoadPropertyInst %#, "right" : string
@@ -2211175,15 +2211145,9 @@ S{" #"#()#} = []
   %# = StoreNewOwnPropertyInst %# : number, %# : object, "translateX" : string, true : boolean
   %# = AllocArrayInst # : number
   %# = StoreOwnPropertyInst %# : object, %# : object, # : number, true : boolean
-  %# = HBCAllocObjectFromBufferInst # : number, "position" : string, "absolute" : string, "height" : string, null : null, "width" : string, null : null, "borderRadius" : string, "full" : string, "backgroundColor" : string, null : null, "borderWidth" : string, # : number
-  %# = StorePropertyInst %# : string, %# : object, "height" : string
-  %# = StorePropertyInst %# : string, %# : object, "width" : string
+  %# = HBCAllocObjectFromBufferInst # : number, "position" : string, "absolute" : string, "height" : string, "#px" : string, "width" : string, "#px" : string, "borderRadius" : string, "full" : string, "backgroundColor" : string, null : null, "borderWidth" : string, # : number, "borderColor" : string, null : null, "borderStyle" : string, "solid" : string, "top" : string, "#%" : string
   %# = StorePropertyInst %#, %# : object, "backgroundColor" : string
-  %# = StoreNewOwnPropertyInst %#, %# : object, "borderColor" : string, true : boolean
-  %# = HBCLoadConstInst "solid" : string
-  %# = StoreNewOwnPropertyInst %# : string, %# : object, "borderStyle" : string, true : boolean
-  %# = HBCLoadConstInst "#%" : string
-  %# = StoreNewOwnPropertyInst %# : string, %# : object, "top" : string, true : boolean
+  %# = StorePropertyInst %#, %# : object, "borderColor" : string
   %# = StoreNewOwnPropertyInst %# : string|number, %# : object, "right" : string, true : boolean
   %# = StoreNewOwnPropertyInst %# : string|number, %# : object, "left" : string, true : boolean
   %# = StoreNewOwnPropertyInst %# : object, %# : object, "transform" : string, true : boolean
@@ -2375489,12 +2375453,9 @@ S{MeterIcon#()#} = []
   %# = HBCCallNInst %#, undefined : undefined, %#, %# : object
   %# = LoadPropertyInst %#, "interpolate" : string
   %# = AllocArrayInst # : number
-  %# = HBCLoadConstInst # : number
-  %# = HBCLoadConstInst "deg" : string
-  %# = BinaryOperatorInst '+', %# : number, %# : string
+  %# = HBCLoadConstInst "#deg" : string
   %# = StoreOwnPropertyInst %# : string, %# : object, # : number, true : boolean
-  %# = HBCLoadConstInst # : number
-  %# = BinaryOperatorInst '+', %# : number, %# : string
+  %# = HBCLoadConstInst "#deg" : string
   %# = StoreOwnPropertyInst %# : string, %# : object, # : number, true : boolean
   %# = AllocObjectInst # : number, empty
   %# = AllocArrayInst # : number, # : number, # : number
@@ -2394361,8 +2394322,7 @@ S{" #"#()#} = []
   %# = LoadPropertyInst %#, "primitive" : string
   %# = LoadPropertyInst %#, "$white" : string
   %# = AllocObjectInst # : number, empty
-  %# = HBCLoadConstInst "px" : string
-  %# = BinaryOperatorInst '+', %# : number, %# : string
+  %# = HBCLoadConstInst "#px" : string
   %# = StoreNewOwnPropertyInst %# : string, %# : object, "height" : string, true : boolean
   %# = StoreNewOwnPropertyInst %#, %# : object, "bgColor" : string, true : boolean
   %# = HBCCallNInst %#, undefined : undefined, %#, %# : object
@@ -2472666,16 +2472626,9 @@ S{" #"#()#} = [_configureTheme#]
   %# = LoadPropertyInst %#, "Box" : string
   %# = LoadPropertyInst %#, "withConfig" : string
   %# = AllocObjectInst # : number, empty
-  %# = HBCLoadConstInst "calc(#% - " : string
-  %# = HBCLoadConstInst # : number
-  %# = BinaryOperatorInst '+', %# : string, %# : number
-  %# = HBCLoadConstInst "px)" : string
-  %# = BinaryOperatorInst '+', %# : string, %# : string
+  %# = HBCLoadConstInst "calc(#% - #px)" : string
   %# = StoreNewOwnPropertyInst %# : string, %# : object, "maxWidth" : string, true : boolean
-  %# = HBCAllocObjectFromBufferInst # : number, "margin" : string, null : null, "_web" : string, null : null, "borderRadius" : string, # : number, "minHeight" : string, "#px" : string, "justifyContent" : string, "center" : string, "alignItems" : string, "center" : string, "flex" : string, # : number
-  %# = HBCLoadConstInst "px" : string
-  %# = BinaryOperatorInst '+', %# : number, %# : string
-  %# = StorePropertyInst %# : string, %# : object, "margin" : string
+  %# = HBCAllocObjectFromBufferInst # : number, "margin" : string, "#px" : string, "_web" : string, null : null, "borderRadius" : string, # : number, "minHeight" : string, "#px" : string, "justifyContent" : string, "center" : string, "alignItems" : string, "center" : string, "flex" : string, # : number
   %# = StorePropertyInst %# : object, %# : object, "_web" : string
   %# = HBCCallNInst %#, undefined : undefined, %#, %# : object
   %# = StorePropertyInst %#, %#, "TileCard" : string

[sebholl]

@tmikov - could I get feedback on any of these PRs? Happy to duplicate to other branches and/or abandon them all if you guys are not in a good place to accept PRs.

[tmikov]

@sebholl sorry, we have been very busy and these PRs require proper review. The matter is complicated because this is supposed to be a frozen branch, so we must review them both for correctness and for forward portability. We haven't forgotten.