From 0e26cfb66eef70be84aaa9c8f560c479fb7bf92f Mon Sep 17 00:00:00 2001 From: whitequark Date: Sat, 22 Aug 2015 13:31:09 -0700 Subject: [PATCH] LocalAccessValidator: relax restrictions to accept def f(); def g(). --- artiq/compiler/module.py | 2 +- .../transforms/dead_code_eliminator.py | 9 ++-- artiq/compiler/validators/local_access.py | 48 +++++++++++++------ lit-test/test/integration/finally.py | 44 ++++++++--------- lit-test/test/local_access/invalid_closure.py | 15 ++++++ .../{invalid.py => invalid_flow.py} | 7 --- 6 files changed, 77 insertions(+), 48 deletions(-) create mode 100644 lit-test/test/local_access/invalid_closure.py rename lit-test/test/local_access/{invalid.py => invalid_flow.py} (66%) diff --git a/artiq/compiler/module.py b/artiq/compiler/module.py index f6285540a..365760bb7 100644 --- a/artiq/compiler/module.py +++ b/artiq/compiler/module.py @@ -60,7 +60,7 @@ class Module: escape_validator.visit(src.typedtree) self.artiq_ir = artiq_ir_generator.visit(src.typedtree) dead_code_eliminator.process(self.artiq_ir) - # local_access_validator.process(self.artiq_ir) + local_access_validator.process(self.artiq_ir) def build_llvm_ir(self, target): """Compile the module to LLVM IR for the specified target.""" diff --git a/artiq/compiler/transforms/dead_code_eliminator.py b/artiq/compiler/transforms/dead_code_eliminator.py index 1159dc0ca..dfe5ccf6c 100644 --- a/artiq/compiler/transforms/dead_code_eliminator.py +++ b/artiq/compiler/transforms/dead_code_eliminator.py @@ -14,10 +14,11 @@ class DeadCodeEliminator: self.process_function(func) def process_function(self, func): - for block in func.basic_blocks: - if not any(block.predecessors()) and \ - not any([isinstance(use, ir.SetLocal) for use in block.uses]) and \ - block != func.entry(): + for block in list(func.basic_blocks): + if not any(block.predecessors()) and block != func.entry(): + for use in set(block.uses): + if isinstance(use, ir.SetLocal): + use.erase() self.remove_block(block) def remove_block(self, block): diff --git a/artiq/compiler/validators/local_access.py b/artiq/compiler/validators/local_access.py index fb68b54d4..156a26e05 100644 --- a/artiq/compiler/validators/local_access.py +++ b/artiq/compiler/validators/local_access.py @@ -16,11 +16,13 @@ class LocalAccessValidator: self.process_function(func) def process_function(self, func): - # Find all environments allocated in this func. - environments = [] + # Find all environments and closures allocated in this func. + environments, closures = [], [] for insn in func.instructions(): if isinstance(insn, ir.Alloc) and ir.is_environment(insn.type): environments.append(insn) + elif isinstance(insn, ir.Closure): + closures.append(insn) # Compute initial state of interesting environments. # Environments consisting only of internal variables (containing a ".") @@ -82,6 +84,7 @@ class LocalAccessValidator: # It's the entry block and it was never initialized. return None + set_local_in_this_frame = False if isinstance(insn, (ir.SetLocal, ir.GetLocal)) and \ "." not in insn.var_name: env, var_name = insn.environment(), insn.var_name @@ -91,24 +94,41 @@ class LocalAccessValidator: if isinstance(insn, ir.SetLocal): # We've just initialized it. block_state[env][var_name] = True + set_local_in_this_frame = True else: # isinstance(insn, ir.GetLocal) if not block_state[env][var_name]: # Oops, accessing it uninitialized. self._uninitialized_access(insn, var_name, pred_at_fault(env, var_name)) - # Creating a closure has no side effects. However, using a closure does. - for operand in insn.operands: - if isinstance(operand, ir.Closure): - env = operand.environment() - # Make sure this environment has any interesting variables. - if env in block_state: - for var_name in block_state[env]: - if not block_state[env][var_name]: - # A closure would capture this variable while it is not always - # initialized. Note that this check is transitive. - self._uninitialized_access(operand, var_name, - pred_at_fault(env, var_name)) + closures_to_check = [] + + if (isinstance(insn, (ir.SetLocal, ir.SetAttr, ir.SetElem)) and + not set_local_in_this_frame): + # Closures may escape via these mechanisms and be invoked elsewhere. + if isinstance(insn.value(), ir.Closure): + closures_to_check.append(insn.value()) + + if isinstance(insn, (ir.Call, ir.Invoke)): + # We can't always trace the flow of closures from point of + # definition to point of call; however, we know that, by transitiveness + # of this analysis, only closures defined in this function can contain + # uninitialized variables. + # + # Thus, enumerate the closures, and check all of them during any operation + # that may eventually result in the closure being called. + closures_to_check = closures + + for closure in closures_to_check: + env = closure.environment() + # Make sure this environment has any interesting variables. + if env in block_state: + for var_name in block_state[env]: + if not block_state[env][var_name]: + # A closure would capture this variable while it is not always + # initialized. Note that this check is transitive. + self._uninitialized_access(closure, var_name, + pred_at_fault(env, var_name)) # Save the state. state[block] = block_state diff --git a/lit-test/test/integration/finally.py b/lit-test/test/integration/finally.py index d418fbbe5..e629602ad 100644 --- a/lit-test/test/integration/finally.py +++ b/lit-test/test/integration/finally.py @@ -13,11 +13,6 @@ def f(): print("f-finally") print("f-out") -# CHECK-L: f-try -# CHECK-L: f-finally -# CHECK-L: f-out -f() - def g(): x = True while x: @@ -29,11 +24,6 @@ def g(): print("g-finally") print("g-out") -# CHECK-L: g-try -# CHECK-L: g-finally -# CHECK-L: g-out -g() - def h(): try: print("h-try") @@ -43,12 +33,6 @@ def h(): print("h-out") return 20 -# CHECK-L: h-try -# CHECK-L: h-finally -# CHECK-NOT-L: h-out -# CHECK-L: h 10 -print("h", h()) - def i(): try: print("i-try") @@ -59,12 +43,6 @@ def i(): print("i-out") return 20 -# CHECK-L: i-try -# CHECK-L: i-finally -# CHECK-NOT-L: i-out -# CHECK-L: i 30 -print("i", i()) - def j(): try: print("j-try") @@ -72,6 +50,28 @@ def j(): print("j-finally") print("j-out") +# CHECK-L: f-try +# CHECK-L: f-finally +# CHECK-L: f-out +f() + +# CHECK-L: g-try +# CHECK-L: g-finally +# CHECK-L: g-out +g() + +# CHECK-L: h-try +# CHECK-L: h-finally +# CHECK-NOT-L: h-out +# CHECK-L: h 10 +print("h", h()) + +# CHECK-L: i-try +# CHECK-L: i-finally +# CHECK-NOT-L: i-out +# CHECK-L: i 30 +print("i", i()) + # CHECK-L: j-try # CHECK-L: j-finally # CHECK-L: j-out diff --git a/lit-test/test/local_access/invalid_closure.py b/lit-test/test/local_access/invalid_closure.py new file mode 100644 index 000000000..75948fb57 --- /dev/null +++ b/lit-test/test/local_access/invalid_closure.py @@ -0,0 +1,15 @@ +# RUN: %python -m artiq.compiler.testbench.signature +diag %s >%t +# RUN: OutputCheck %s --file-to-check=%t + +if False: + t = 1 + +# CHECK-L: ${LINE:+1}: error: variable 't' can be captured in a closure uninitialized +l = lambda: t + +# CHECK-L: ${LINE:+1}: error: variable 't' can be captured in a closure uninitialized +def f(): + return t + +l() +f() diff --git a/lit-test/test/local_access/invalid.py b/lit-test/test/local_access/invalid_flow.py similarity index 66% rename from lit-test/test/local_access/invalid.py rename to lit-test/test/local_access/invalid_flow.py index fabd6e9a8..7b99958b4 100644 --- a/lit-test/test/local_access/invalid.py +++ b/lit-test/test/local_access/invalid_flow.py @@ -18,10 +18,3 @@ else: t = 1 # CHECK-L: ${LINE:+1}: error: variable 't' is not always initialized -t - -# CHECK-L: ${LINE:+1}: error: variable 't' can be captured in a closure uninitialized -l = lambda: t - -# CHECK-L: ${LINE:+1}: error: variable 't' can be captured in a closure uninitialized -def f(): - return t