Repository navigation
Allow arbitrary walk-back in reduction nodes to find inplace_binop. - #7349
Conversation
… we find a non-assignment.
|
Many thanks for the PR! I've tested with the reproducer from #7344, and this does appear to resolve the issue. |
|
@stuartarchibald or @gmarkall On how many cores does CI currently run? The test for this issue will only be effective with 2+ cores. Just want to make sure we have at least that many. |
|
The public CI runs on Azure Pipelines which, according to this, get 2 cores for Windows and Linux environments, and 3 cores for macOS. The internal CI system has at least 4 cores for all test environments. |
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch, the fix does make the original MWR work correctly but there seems to be a similar issue in a variant. Thanks for taking a look at it!
| cur_index = 1 | ||
| acc_expr = nodes[-cur_index].value | ||
| while isinstance(acc_expr, ir.Var): | ||
| require(len(nodes) >= cur_index + 1) | ||
| require(nodes[-(cur_index + 1)].target.name == nodes[-cur_index].value.name) | ||
| cur_index += 1 | ||
| acc_expr = nodes[-cur_index].value |
There was a problem hiding this comment.
It seems like this might be relying on a naming pattern? The following variant on the original MWR breaks for variable t:
from numba import njit, prange
import numpy as np
@njit(parallel=True)
def test_impl(A, cond):
s = 1
t = 10
for i in prange(A.shape[0]):
if cond[i]:
s += 1
t += 1
else:
s += 2
return s, t
print(test_impl(np.ones(10), (np.arange(10) % 2).astype('bool')))
stuartarchibald
added
4 - Waiting on author
…s only incremented in the if part of an if/else.
stuartarchibald
added
4 - Waiting on reviewer
stuartarchibald
left a comment
stuartarchibald
left a comment
There was a problem hiding this comment.
Thanks for the patch and fixes.
stuartarchibald
added
5 - Ready to merge
5 participants
Resolves #7344
Previously, there could only be one extra assignment after the inplace_binop node for a reduction before the assignment to the reduction variable. In this issue, there was an additional assignment. The solution is to start at the enf with the reduction variable and walk backwards through the reduce nodes so long as they are all assignments and link with each other until we get to inplace_binop.