Repository: numba/numba
Pull request: Support binop reduction.
Resolves #9518.


Problem statement:
numba's bug with prange without warning
(Edited by maintainer to inline file attachment)

Here is a minimal code to demonstrate a bug I found.This code can run smoothly without error and warning,
but it gives an unreasonable result, with output number ‘err1’ equals ‘err2’.However, ‘err1’ and ‘err2’ should be of opposite sign. Bug code see the attachment

```python
import numba
import time
from numba import jit
from numba import prange
import numpy as np
test=np.arange(10000).reshape((100,100))

@jit(nopython=True,parallel=True)
def bug(test,pre,err1,err2):
	for k in range(0,100):
		pre=pre+test[k,0]
	for k in prange(0,100):
		err1=err1+test[k,0]
		err2=err2-test[k,0]
	return(err1,err2)

err1,err2=bug(test,0,0,0)
print(err1,err2)

```

I found there are two ways to overcome the bug.

1.not use prange
2.Delete the first loop at line 10 and line11
     But, still I’d like to know how the bug arises,in case of bugs appear without warning
 next time. By the way, Could you please send a copy of your responce to my email baiyunfei@iphy.ac.cn
Lookinng forward to your reply.
Yours sincerely

Hints from issue discussion:
It seems that parfors is mishandling the reduction when it is written as `err2=err2-test[k,0]` but works when it's written as `err2 -= test[k,0]`.

(CC @DrTodd13)

`get_parfor_reductions()` is not setting `redop` when the reduction is not written with an inplace binop; e.g ` err2=err2-test[k,0]`. As a result, `_parfor_lowering_finalize_reduction()` calls `_lower_non_trivial_reduce()` and emitting 

```
$err2.2.23 = err2.2 - err2.2#init
```
which is `0 -  (-495000)`.

Should `get_parfor_reductions()` have set `redop` to `operator.isub` instead?

> (CC @DrTodd13)
> 
> `get_parfor_reductions()` is not setting `redop` when the reduction is not written with an inplace binop; e.g ` err2=err2-test[k,0]`. As a result, `_parfor_lowering_finalize_reduction()` calls `_lower_non_trivial_reduce()` and emitting
> 
> ```
> $err2.2.23 = err2.2 - err2.2#init
> ```
> 
> which is `0 - (-495000)`.
> 
> Should `get_parfor_reductions()` have set `redop` to `operator.isub` instead?

I suppose this is common enough that we should recognize the pattern a = a - b (or a = a + b, etc.).  Storing those internally as isub (or iadd, etc) may work.  There is code in there for isub (parfor_lowering:581) such that the reduction across the individual thread results is done with addition.    I suppose in this case it doesn't know to do that for a=a-b and so it does subtraction across the threads which ends up adding due to the negative.

Commit message: Update numba/core/ir.py

Co-authored-by: Siu Kwan Lam <1929845+sklam@users.noreply.github.com>

Patch to review:
diff --git a/docs/upcoming_changes/9521.new_feature.rst b/docs/upcoming_changes/9521.new_feature.rst
new file mode 100644
index 00000000000..1ce0326a97d
--- /dev/null
+++ b/docs/upcoming_changes/9521.new_feature.rst
@@ -0,0 +1,4 @@
+Add support for parfor binop reductions.
+----------------------------------------
+
+Previously, only operations with inplace operations like += could be used as reductions in pranges.  Now, with this PR, binop reductions of the form a = a binop b can be used.
diff --git a/numba/core/ir.py b/numba/core/ir.py
index c19312746d8..a78745311f9 100644
--- a/numba/core/ir.py
+++ b/numba/core/ir.py
@@ -336,6 +336,12 @@ def _rec_list_vars(self, val):
         else:
             return []
 
+    def __repr__(self):
+        return self.__str__()
+
+    def __str__(self):
+        return super().__repr__()
+
 
 class Stmt(Inst):
     """
diff --git a/numba/parfors/parfor.py b/numba/parfors/parfor.py
index cd862809ffd..3c71d4c3f20 100644
--- a/numba/parfors/parfor.py
+++ b/numba/parfors/parfor.py
@@ -3803,12 +3803,22 @@ def get_reduction_init(nodes):
     # there could be multiple extra assignments after the reduce node
     # See: test_reduction_var_reuse
     acc_expr = list(filter(lambda x: isinstance(x.value, ir.Expr), nodes))[-1].value
-    require(isinstance(acc_expr, ir.Expr) and acc_expr.op=='inplace_binop')
-    if acc_expr.fn == operator.iadd or acc_expr.fn == operator.isub:
-        return 0, acc_expr.fn
-    if (  acc_expr.fn == operator.imul
-       or acc_expr.fn == operator.itruediv ):
-        return 1, acc_expr.fn
+    require(isinstance(acc_expr, ir.Expr) and acc_expr.op in ['inplace_binop', 'binop'])
+    acc_expr_fn = acc_expr.fn
+    if acc_expr.op == 'binop':
+        if acc_expr_fn == operator.add:
+            acc_expr_fn = operator.iadd
+        elif acc_expr_fn == operator.sub:
+            acc_expr_fn = operator.isub
+        elif acc_expr_fn == operator.mul:
+            acc_expr_fn = operator.imul
+        elif acc_expr_fn == operator.truediv:
+            acc_expr_fn = operator.itruediv
+    if acc_expr_fn == operator.iadd or acc_expr_fn == operator.isub:
+        return 0, acc_expr_fn
+    if (  acc_expr_fn == operator.imul
+       or acc_expr_fn == operator.itruediv ):
+        return 1, acc_expr_fn
     return None, None
 
 def supported_reduction(x, func_ir):
diff --git a/numba/parfors/parfor_lowering.py b/numba/parfors/parfor_lowering.py
index 5f2b9f3c98e..f31d67cb82d 100644
--- a/numba/parfors/parfor_lowering.py
+++ b/numba/parfors/parfor_lowering.py
@@ -473,7 +473,14 @@ def _lower_trivial_inplace_binops(parfor, lowerer, thread_count_var, reduce_info
         if _lower_var_to_var_assign(lowerer, inst):
             pass
         # Is inplace-binop for the reduction?
-        elif _is_inplace_binop_and_rhs_is_init(inst, reduce_info.redvar_name):
+        elif _is_right_op_and_rhs_is_init(inst, reduce_info.redvar_name, "inplace_binop"):
+            fn = inst.value.fn
+            redvar_result = _emit_binop_reduce_call(
+                fn, lowerer, thread_count_var, reduce_info,
+            )
+            lowerer.storevar(redvar_result, name=inst.target.name)
+        # Is binop for the reduction?
+        elif _is_right_op_and_rhs_is_init(inst, reduce_info.redvar_name, "binop"):
             fn = inst.value.fn
             redvar_result = _emit_binop_reduce_call(
                 fn, lowerer, thread_count_var, reduce_info,
@@ -579,9 +586,14 @@ def reduction_mul(thread_count, redarr, init):
     kernel = {
         operator.iadd: reduction_add,
         operator.isub: reduction_add,
+        operator.add: reduction_add,
+        operator.sub: reduction_add,
         operator.imul: reduction_mul,
         operator.ifloordiv: reduction_mul,
         operator.itruediv: reduction_mul,
+        operator.mul: reduction_mul,
+        operator.floordiv: reduction_mul,
+        operator.truediv: reduction_mul,
     }[binop]
 
     ctx = lowerer.context
@@ -612,7 +624,7 @@ def reduction_mul(thread_count, redarr, init):
     return redvar_result
 
 
-def _is_inplace_binop_and_rhs_is_init(inst, redvar_name):
+def _is_right_op_and_rhs_is_init(inst, redvar_name, op):
     """Is ``inst`` an inplace-binop and the RHS is the reduction init?
     """
     if not isinstance(inst, ir.Assign):
@@ -620,7 +632,7 @@ def _is_inplace_binop_and_rhs_is_init(inst, redvar_name):
     rhs = inst.value
     if not isinstance(rhs, ir.Expr):
         return False
-    if rhs.op != "inplace_binop":
+    if rhs.op != op:
         return False
     if rhs.rhs.name != f"{redvar_name}#init":
         return False
diff --git a/numba/tests/test_parfors.py b/numba/tests/test_parfors.py
index a308c9490a3..c8b57cc1a6b 100644
--- a/numba/tests/test_parfors.py
+++ b/numba/tests/test_parfors.py
@@ -3466,6 +3466,20 @@ def test_impl():
         diagnostics = cpfunc.metadata['parfor_diagnostics']
         self.assert_diagnostics(diagnostics, parfors_count=2)
 
+    def test_reduction_binop(self):
+        def test_impl():
+            n = 10
+            a = np.ones(n + 1) # prevent fusion
+            acc = 0
+            for i in prange(n):
+                acc = acc - a[i]
+            return acc
+
+        self.check(test_impl,)
+        cpfunc = self.compile_parallel(test_impl, ())
+        diagnostics = cpfunc.metadata['parfor_diagnostics']
+        self.assert_diagnostics(diagnostics, parfors_count=2)
+
     def test_setitem(self):
         def test_impl():
             n = 10


Code review comments:
diff_hunk: @@ -336,6 +336,12 @@ def _rec_list_vars(self, val):
         else:
             return []
 
+    def __repr__(self):
+        return self.__str__()
+
+    def __str__(self):
+        return super().__repr__()
+
path: numba/core/ir.py
text: @user1:
My [suggestion](https://github.com/numba/numba/pull/9521#discussion_r1559923460) is leading to other test failures since the string repr behavior changed. @author, maybe it is easier just to remove the these changes all together, assuming it is just needed for debugging. 

@author:
Done. @user1