Hoisting logic needs to consider dependency on prior unhoistable variable #9586
Conversation
…cy on prior unhoistable variable.
numba/parfors/parfor_lowering.py
Outdated
| unhoistable = {assgn.target.name for assgn, _reason in not_hoisted} | ||
| use_unhoist = uses & unhoistable | ||
| diff = uses.difference(dep_on_param) | ||
| diff |= use_unhoist |
There was a problem hiding this comment.
@DrTodd13, I need your expertise here. I only did the smallest change I can think of.
The origin is that a static_getitem(array, index) is referring to an array that is previously placed into not_hoisted. The source code looks like:
for .. in loop:
array = solve(arg)
res = array[1] # the static getitem
if compute(res):
break
use(array)Pre-patch logic will cause that static_getitem to be hoisted out of the loop. I changed it to look into the list of not_hoisted and ban hoisting on uses that refer to it.
@sklam |
Co-authored-by: Todd A. Anderson <[email protected]>
sklam
added
4 - Waiting on second reviewer
| @@ -47,8 +47,9 @@ def stable_fit(X, y, threshold=3): | |||
| rmse = np.sqrt(np.mean(resid_sub ** 2)) | |||
| first = np.fabs(resid_sub[0]) / rmse < threshold | |||
| last = np.fabs(resid_sub[-1]) / rmse < threshold | |||
| slope = np.fabs(beta_sub[1]) / rmse < threshold | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Without the fix in parfor_lowering, the modified test case fails with:
test_issue9490_non_det_ssa_problem (numba.tests.test_parfors.TestParfors.test_issue9490_non_det_ssa_problem) ... FAIL
======================================================================
FAIL: test_issue9490_non_det_ssa_problem (numba.tests.test_parfors.TestParfors.test_issue9490_non_det_ssa_problem)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/gmarkall/numbadev/numba/numba/tests/test_parfors.py", line 2397, in test_issue9490_non_det_ssa_problem
subp.check_output(cmd, env=envs,
File "/home/gmarkall/miniforge3/envs/numbadev/lib/python3.11/subprocess.py", line 466, in check_output
return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/home/gmarkall/miniforge3/envs/numbadev/lib/python3.11/subprocess.py", line 571, in run
raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['/home/gmarkall/miniforge3/envs/numbadev/bin/python', '-m', 'numba.tests.parfor_iss9490_usecase']' died with <Signals.SIGSEGV: 11>.
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/gmarkall/numbadev/numba/numba/tests/test_parfors.py", line 2402, in test_issue9490_non_det_ssa_problem
self.fail(msg=msg)
AssertionError: subprocess failed with output:
Fatal Python error: Segmentation fault
Current thread 0x00007178c7191740 (most recent call first):
File "/home/gmarkall/numbadev/numba/numba/tests/parfor_iss9490_usecase.py", line 69 in check
File "/home/gmarkall/numbadev/numba/numba/tests/parfor_iss9490_usecase.py", line 78 in <module>
File "<frozen runpy>", line 88 in _run_code
File "<frozen runpy>", line 198 in _run_module_as_main
and the reproducer from #9581 also segfaults.
With the change in this PR, the test case passes and the reproducer from #9581 executes to completion.
I don't have enough knowledge of parfors to have an opinion on the fix, but from a "works for me" perspective, this PR appears to do the job.
esc
added
5 - Ready to merge
Hoisting logic needs to consider dependency on prior unhoistable variable
Fix #9581, #9490.