Remove hack

toitlang · Oct 23, 2024 · bdede84 · bdede84
1 parent 6ef6018
commit bdede84
Show file tree

Hide file tree

Showing 7 changed files with 149 additions and 41 deletions.
diff --git a/src/compiler/propagation/type_propagator.cc b/src/compiler/propagation/type_propagator.cc
@@ -372,35 +372,11 @@ void TypePropagator::call_method(MethodTemplate* caller,
                                  TypeScope* scope,
                                  uint8* site,
                                  Method target,
-                                 std::vector<ConcreteType>& arguments,
-                                 std::vector<Worklist*>& worklists) {
+                                 std::vector<ConcreteType>& arguments) {
   TypeStack* stack = scope->top();
   int arity = target.arity();
   int index = arguments.size();
   if (index == arity) {
-    // Collect all block arguments. We cannot handle block arguments
-    // outside of ordinary methods, so if the caller isn't set, we
-    // analyzing a call to a method from an entry stub of sorts.
-    if (caller && !hacko_) {
-      Set<BlockTemplate*> blocks;
-      for (auto it : arguments) {
-        if (!it.is_block()) continue;
-        BlockTemplate* block = it.block();
-        bool own = block->origin()->method().header_bcp() == caller->method().header_bcp();
-        if (own) blocks.insert(block);
-      }
-      if (!blocks.empty()) {
-        bool changed = true;
-        while (changed) {
-          changed = false;
-          for (BlockTemplate* block : blocks) {
-            changed = block->propagate(scope, worklists, block->is_invoked_from_try_block()) || changed;
-          }
-        }
-      }
-      hacko_ = true;
-    }
-
     MethodTemplate* callee = find_method(target, arguments);
     // TODO(kasper): Analyzing the callee method eagerly while still
     // in the process of analyzing the caller is an optimization. It
@@ -426,27 +402,57 @@ void TypePropagator::call_method(MethodTemplate* caller,
   if (type.is_block()) {
     BlockTemplate* block = type.block();
     arguments.push_back(block->pass_as_argument(scope));
-    call_method(caller, scope, site, target, arguments, worklists);
+    call_method(caller, scope, site, target, arguments);
     arguments.pop_back();
   } else if (type.size(words_per_type_) > 5) {
     // If one of the arguments is megamorphic, we analyze the target
     // method with the any type for that argument instead. This cuts
     // down on the number of separate analysis at the cost of more
     // mixing of types and worse propagated types.
     arguments.push_back(ConcreteType::any());
-    call_method(caller, scope, site, target, arguments, worklists);
+    call_method(caller, scope, site, target, arguments);
     arguments.pop_back();
   } else {
     TypeSet::Iterator it(type, words_per_type_);
     while (it.has_next()) {
       unsigned id = it.next();
       arguments.push_back(ConcreteType(id));
-      call_method(caller, scope, site, target, arguments, worklists);
+      call_method(caller, scope, site, target, arguments);
       arguments.pop_back();
     }
   }
 }
 
+void TypePropagator::propagate_blocks(MethodTemplate* caller,
+                                     TypeScope* scope,
+                                     int arity,
+                                     std::vector<Worklist*>& worklists) {
+  // Propagate types through all locally-defined block arguments.
+  // We cannot handle block arguments outside of ordinary methods,
+  // so if the caller isn't set, we analyzing a call to a method from
+  // an entry stub of sorts.
+  if (!caller) return;
+
+  Set<BlockTemplate*> blocks;
+  TypeStack* stack = scope->top();
+  for (int i = 0; i < arity; i++) {
+    TypeSet type = stack->local(arity - i - 1);
+    if (!type.is_block()) continue;
+    BlockTemplate* block = type.block();
+    // The block is our own if it is defined inside the same method as the caller.
+    bool own = block->origin()->method().header_bcp() == caller->method().header_bcp();
+    if (own) blocks.insert(block);
+  }
+
+  bool changed = !blocks.empty();
+  while (changed) {
+    changed = false;
+    for (BlockTemplate* block : blocks) {
+      changed = block->propagate(scope, worklists, block->is_invoked_from_try_block()) || changed;
+    }
+  }
+}
+
 void TypePropagator::call_static(MethodTemplate* caller,
                                  TypeScope* scope,
                                  uint8* site,
@@ -456,7 +462,7 @@ void TypePropagator::call_static(MethodTemplate* caller,
   int arity = target.arity();
   if (site) add_input(site, stack, arity);
 
-  hacko_ = false;
+  propagate_blocks(caller, scope, arity, worklists);
   std::vector<ConcreteType> arguments;
   stack->push_empty();
 
@@ -474,7 +480,7 @@ void TypePropagator::call_static(MethodTemplate* caller,
   }
 
   if (handle_as_static) {
-    call_method(caller, scope, site, target, arguments, worklists);
+    call_method(caller, scope, site, target, arguments);
   } else {
     // We're handling this as a call to a virtual method,
     // but if the offset is negative it indirectly encodes
@@ -515,7 +521,7 @@ void TypePropagator::call_static(MethodTemplate* caller,
         continue;
       }
       arguments.push_back(ConcreteType(id));
-      call_method(caller, scope, site, target, arguments, worklists);
+      call_method(caller, scope, site, target, arguments);
       arguments.pop_back();
     }
   }
@@ -533,7 +539,7 @@ void TypePropagator::call_virtual(MethodTemplate* caller,
   TypeSet receiver = stack->local(arity - 1);
   if (site) add_input(site, stack, arity);
 
-  hacko_ = false;
+  propagate_blocks(caller, scope, arity, worklists);
   std::vector<ConcreteType> arguments;
   stack->push_empty();
 
@@ -553,7 +559,7 @@ void TypePropagator::call_virtual(MethodTemplate* caller,
       continue;
     }
     arguments.push_back(ConcreteType(id));
-    call_method(caller, scope, site, target, arguments, worklists);
+    call_method(caller, scope, site, target, arguments);
     arguments.pop_back();
   }
 
@@ -1434,6 +1440,10 @@ static TypeScope* process(TypeScope* scope, uint8* bcp, std::vector<Worklist*>&
   OPCODE_END();
 
   OPCODE_BEGIN(UNWIND);
+    // Here we continue unwinding if an exception was thrown, so
+    // we must make this as a potential throw site to make sure
+    // any modifications to outer locals are merged back.
+    scope->throw_maybe();
     // If the try-block is guaranteed to cause unwinding,
     // we avoid analyzing the bytecodes following this one.
     TypeSet target = stack->local(1);

diff --git a/src/compiler/propagation/type_propagator.h b/src/compiler/propagation/type_propagator.h
@@ -82,8 +82,6 @@ class TypePropagator {
   Program* const program_;
   int words_per_type_;
 
-  bool hacko_ = false;
-
 #define HAS_ENTRY_POINT(name, symbol, arity) \
   bool has_##name##_ = false;
   ENTRY_POINTS(HAS_ENTRY_POINT)
@@ -104,8 +102,12 @@ class TypePropagator {
                    TypeScope* scope,
                    uint8* site,
                    Method target,
-                   std::vector<ConcreteType>& arguments,
-                   std::vector<Worklist*>& worklists);
+                   std::vector<ConcreteType>& arguments);
+
+  void propagate_blocks(MethodTemplate* caller,
+                       TypeScope* scope,
+                       int arity,
+                       std::vector<Worklist*>& worklists);
 
   MethodTemplate* find_method(Method target, std::vector<ConcreteType> arguments);
   BlockTemplate* find_block(MethodTemplate* origin, Method method, int level, int sp);

diff --git a/tests/type_propagation/block-test.toit b/tests/type_propagation/block-test.toit
@@ -12,6 +12,7 @@ main:
   test-modify-outer-nested
   test-recursion
   test-dead
+  test-dependent
 
 test-simple:
   x := 0
@@ -106,6 +107,15 @@ test-dead:
   ignore: it
   ignore: | x y | 42
 
+test-dependent:
+  x := "hest"
+  y := null
+  invoke
+      (: x = y )
+      (: y = 42 )
+  id x
+  id y
+
 recursive-null [block]:
   if pick: return recursive-null: null
   return block.call
@@ -143,5 +153,9 @@ pick:
 invoke [block]:
   return block.call
 
+invoke [b1] [b2] -> none:
+  b1.call
+  b2.call
+
 invoke x [block]:
   return block.call x
diff --git a/tests/type_propagation/gold/block-test.gold b/tests/type_propagation/gold/block-test.gold
@@ -16,7 +16,9 @@ main tests/type_propagation/block-test.toit
  28[053] - invoke static test-recursion tests/type_propagation/block-test.toit // {Null_}
  31[041] - pop 1
  32[053] - invoke static test-dead tests/type_propagation/block-test.toit // {Null_}
- 35[090] - return null S1 0
+ 35[041] - pop 1
+ 36[053] - invoke static test-dependent tests/type_propagation/block-test.toit // {Null_}
+ 39[090] - return null S1 0
 
 test-simple tests/type_propagation/block-test.toit
   0[023] - load smi 0
@@ -405,6 +407,36 @@ test-dead tests/type_propagation/block-test.toit
  31[053] - invoke static ignore tests/type_propagation/block-test.toit // [[block]] -> {Null_}
  34[090] - return null S2 0
 
+test-dependent tests/type_propagation/block-test.toit
+  0[020] - load literal hest
+  2[022] - load null
+  3[029] - load method [block] in test-dependent tests/type_propagation/block-test.toit
+  8[029] - load method [block] in test-dependent tests/type_propagation/block-test.toit
+ 13[038] - load block 1
+ 15[038] - load block 1
+ 17[053] - invoke static invoke tests/type_propagation/block-test.toit // [[block], [block]] -> {Null_}
+ 20[040] - pop 3
+ 22[015] - load local 1
+ 23[053] - invoke static id tests/type_propagation/block-test.toit // [{String_|Null_|SmallInteger_}] -> {String_|Null_|SmallInteger_}
+ 26[002] - pop, load local S0
+ 28[053] - invoke static id tests/type_propagation/block-test.toit // [{Null_|SmallInteger_}] -> {Null_|SmallInteger_}
+ 31[090] - return null S3 0
+
+[block] in test-dependent tests/type_propagation/block-test.toit
+ - argument 0: [block]
+  0[016] - load local 2
+  1[017] - load local 3
+  2[005] - load outer S1 // {Null_|SmallInteger_}
+  4[006] - store outer S2
+  6[089] - return S1 1
+
+[block] in test-dependent tests/type_propagation/block-test.toit
+ - argument 0: [block]
+  0[016] - load local 2
+  1[026] - load smi 42
+  3[006] - store outer S2
+  5[089] - return S1 1
+
 recursive-null tests/type_propagation/block-test.toit
  - argument 0: [block]
   0[053] - invoke static pick tests/type_propagation/block-test.toit // {True|False}
@@ -507,6 +539,15 @@ invoke tests/type_propagation/block-test.toit
   1[055] - invoke block S1 // [[block]] -> {String_|Null_|True|float|SmallInteger_}
   3[089] - return S1 1
 
+invoke tests/type_propagation/block-test.toit
+ - argument 0: [block]
+ - argument 1: [block]
+  0[017] - load local 3
+  1[055] - invoke block S1 // [[block]] -> {Null_|SmallInteger_}
+  3[002] - pop, load local S2
+  5[055] - invoke block S1 // [[block]] -> {SmallInteger_}
+  7[090] - return null S1 2
+
 invoke tests/type_propagation/block-test.toit
  - argument 0: {String_|True|SmallInteger_}
  - argument 1: [block]

diff --git a/tests/type_propagation/gold/block-test.gold-O2 b/tests/type_propagation/gold/block-test.gold-O2
@@ -16,7 +16,9 @@ main tests/type_propagation/block-test.toit
  28[053] - invoke static test-recursion tests/type_propagation/block-test.toit // {Null_}
  31[041] - pop 1
  32[053] - invoke static test-dead tests/type_propagation/block-test.toit // {Null_}
- 35[090] - return null S1 0
+ 35[041] - pop 1
+ 36[053] - invoke static test-dependent tests/type_propagation/block-test.toit // {Null_}
+ 39[090] - return null S1 0
 
 test-simple tests/type_propagation/block-test.toit
   0[023] - load smi 0
@@ -403,6 +405,36 @@ test-dead tests/type_propagation/block-test.toit
  31[053] - invoke static ignore tests/type_propagation/block-test.toit // [[block]] -> {Null_}
  34[090] - return null S2 0
 
+test-dependent tests/type_propagation/block-test.toit
+  0[020] - load literal hest
+  2[022] - load null
+  3[029] - load method [block] in test-dependent tests/type_propagation/block-test.toit
+  8[029] - load method [block] in test-dependent tests/type_propagation/block-test.toit
+ 13[038] - load block 1
+ 15[038] - load block 1
+ 17[053] - invoke static invoke tests/type_propagation/block-test.toit // [[block], [block]] -> {Null_}
+ 20[040] - pop 3
+ 22[015] - load local 1
+ 23[053] - invoke static id tests/type_propagation/block-test.toit // [{String_|Null_|SmallInteger_}] -> {String_|Null_|SmallInteger_}
+ 26[002] - pop, load local S0
+ 28[053] - invoke static id tests/type_propagation/block-test.toit // [{Null_|SmallInteger_}] -> {Null_|SmallInteger_}
+ 31[090] - return null S3 0
+
+[block] in test-dependent tests/type_propagation/block-test.toit
+ - argument 0: [block]
+  0[016] - load local 2
+  1[017] - load local 3
+  2[005] - load outer S1 // {Null_|SmallInteger_}
+  4[006] - store outer S2
+  6[089] - return S1 1
+
+[block] in test-dependent tests/type_propagation/block-test.toit
+ - argument 0: [block]
+  0[016] - load local 2
+  1[026] - load smi 42
+  3[006] - store outer S2
+  5[089] - return S1 1
+
 recursive-null tests/type_propagation/block-test.toit
  - argument 0: [block]
   0[053] - invoke static pick tests/type_propagation/block-test.toit // {True|False}
@@ -505,6 +537,15 @@ invoke tests/type_propagation/block-test.toit
   1[055] - invoke block S1 // [[block]] -> {String_|Null_|True|float|SmallInteger_}
   3[089] - return S1 1
 
+invoke tests/type_propagation/block-test.toit
+ - argument 0: [block]
+ - argument 1: [block]
+  0[017] - load local 3
+  1[055] - invoke block S1 // [[block]] -> {Null_|SmallInteger_}
+  3[002] - pop, load local S2
+  5[055] - invoke block S1 // [[block]] -> {SmallInteger_}
+  7[090] - return null S1 2
+
 invoke tests/type_propagation/block-test.toit
  - argument 0: {String_|True|SmallInteger_}
  - argument 1: [block]

diff --git a/tests/type_propagation/gold/finally-test.gold b/tests/type_propagation/gold/finally-test.gold
@@ -217,7 +217,7 @@ test-throw-update-in-finally tests/type_propagation/finally-test.toit
   9[053] - invoke static invoke-catch tests/type_propagation/finally-test.toit // [[block]] -> {Null_}
  12[041] - pop 1
  13[002] - pop, load local S0
- 15[053] - invoke static id tests/type_propagation/finally-test.toit // [{False}] -> {False}
+ 15[053] - invoke static id tests/type_propagation/finally-test.toit // [{True|False}] -> {True|False}
  18[090] - return null S2 0
 
 [block] in test-throw-update-in-finally tests/type_propagation/finally-test.toit

diff --git a/tests/type_propagation/gold/finally-test.gold-O2 b/tests/type_propagation/gold/finally-test.gold-O2
@@ -217,7 +217,7 @@ test-throw-update-in-finally tests/type_propagation/finally-test.toit
   9[053] - invoke static invoke-catch tests/type_propagation/finally-test.toit // [[block]] -> {Null_}
  12[041] - pop 1
  13[002] - pop, load local S0
- 15[053] - invoke static id tests/type_propagation/finally-test.toit // [{False}] -> {False}
+ 15[053] - invoke static id tests/type_propagation/finally-test.toit // [{True|False}] -> {True|False}
  18[090] - return null S2 0
 
 [block] in test-throw-update-in-finally tests/type_propagation/finally-test.toit