Repository: cython/cython
Pull request: Add `if` parameter to `parallel()` and `prange()`
Fixes #5393

> Note: documentation needs to be updated too


Problem statement:
[ENH] parallel/prange(..., if=N>100) and friends
### Is your feature request related to a problem? Please describe.

In my code, I would like to only create parallel/openmp regions for simple/loop constructs with a certain size.  
For instance the overhead of calling a loop construct with 10 elements (with very little work on each iteration) can be quite big.
This is already enabled by the openmp standard since 2
```cython
with parallel(if=N>1000):
    ... code as normal

# add use case related code here
for i in prange(N, if=N>1000):
    ... code as normal
```

This has been in the OpenMP standard since 1.0 and is a simple:
```c
#pragma omp parallel if(<condition>)
...
```
so shouldn't be too hard since everything will be handled by the compiler when setting up the parallel regions.

### Describe the solution you'd like.

The only problem is that `if` is a reserved keyword, so it should be named differently:
- `par_if`
- `IF`
- `when`
- ...?


### Describe alternatives you've considered.

_No response_

### Additional context

I have tried to look into the Cython code to see if I could add the pragma declaration, but it was sadly a bit too hard for me... :(

Hints from issue discussion:
This sounds reasonable. It's probably medium difficulty to implement. I think we'd just call the argument `if_` - that sort of name is fairly common elsewhere in the Python world.
Sounds great!
Don't read that as a promise to do it though! Just that it sounds like a nice feature if someone did do it!
I actually managed to do something, could somebody tell me if I am on the right track:
```patch
diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py
index 877e7b928..35547dc8b 100644
--- a/Cython/Compiler/Nodes.py
+++ b/Cython/Compiler/Nodes.py
@@ -8981,7 +8981,7 @@ class ParallelStatNode(StatNode, ParallelNode):
                                 construct (replaced by its compile time value)
     """
 
-    child_attrs = ['body', 'num_threads']
+    child_attrs = ['body', 'num_threads', 'if_']
 
     body = None
 
@@ -8992,6 +8992,7 @@ class ParallelStatNode(StatNode, ParallelNode):
 
     num_threads = None
     chunksize = None
+    if_ = None
 
     parallel_exc = (
         Naming.parallel_exc_type,
@@ -9034,9 +9035,10 @@ class ParallelStatNode(StatNode, ParallelNode):
         self.body.analyse_declarations(env)
 
         self.num_threads = None
+        self.if_ = None
 
         if self.kwargs:
-            # Try to find num_threads and chunksize keyword arguments
+            # Try to find num_threads, if_ and chunksize keyword arguments
             pairs = []
             seen = set()
             for dictitem in self.kwargs.key_value_pairs:
@@ -9046,6 +9048,9 @@ class ParallelStatNode(StatNode, ParallelNode):
                 if dictitem.key.value == 'num_threads':
                     if not dictitem.value.is_none:
                         self.num_threads = dictitem.value
+                elif dictitem.key.value == 'if_':
+                    if not dictitem.value.is_none:
+                        self.if_ = dictitem.value
                 elif self.is_prange and dictitem.key.value == 'chunksize':
                     if not dictitem.value.is_none:
                         self.chunksize = dictitem.value
@@ -9072,6 +9077,12 @@ class ParallelStatNode(StatNode, ParallelNode):
         if self.num_threads:
             self.num_threads = self.num_threads.analyse_expressions(env)
 
+        if self.if_:
+            if self.is_parallel:
+                self.if_ = self.if_.analyse_expressions(env)
+            else:
+                error(self.pos, "if_ must de declared in the parent parallel section")
+
         if self.chunksize:
             self.chunksize = self.chunksize.analyse_expressions(env)
 
@@ -9091,6 +9102,7 @@ class ParallelStatNode(StatNode, ParallelNode):
             if not self.num_threads.is_simple() or self.num_threads.type.is_pyobject:
                 self.num_threads = self.num_threads.coerce_to(
                     PyrexTypes.c_int_type, env).coerce_to_temp(env)
+
         return self
 
     def analyse_sharing_attributes(self, env):
@@ -9246,6 +9258,12 @@ class ParallelStatNode(StatNode, ParallelNode):
         if self.num_threads is not None:
             code.put(" num_threads(%s)" % self.evaluate_before_block(code, self.num_threads))
 
+    def put_if_(self, code):
+        """
+        Write self.if_ if set as the if OpenMP directive
+        """
+        if self.if_ is not None:
+            code.put(" if(%s)" % self.evaluate_before_block(code, self.if_))
 
     def declare_closure_privates(self, code):
         """
@@ -9681,9 +9699,10 @@ class ParallelWithBlockNode(ParallelStatNode):
     This node represents a 'with cython.parallel.parallel():' block
     """
 
-    valid_keyword_arguments = ['num_threads']
+    valid_keyword_arguments = ['num_threads', 'if_']
 
     num_threads = None
+    if_ = None
 
     def analyse_declarations(self, env):
         super(ParallelWithBlockNode, self).analyse_declarations(env)
@@ -9698,6 +9717,9 @@ class ParallelWithBlockNode(ParallelStatNode):
         code.putln("#ifdef _OPENMP")
         code.put("#pragma omp parallel ")
 
+        # Add if statement, if present
+        self.put_if_(code)
+
         if self.privates:
             privates = [e.cname for e in self.privates
                         if not e.type.is_pyobject]
@@ -9750,7 +9772,7 @@ class ParallelRangeNode(ParallelStatNode):
     nogil = None
     schedule = None
 
-    valid_keyword_arguments = ['schedule', 'nogil', 'num_threads', 'chunksize']
+    valid_keyword_arguments = ['schedule', 'nogil', 'num_threads', 'chunksize', 'if_']
 
     def __init__(self, pos, **kwds):
         super(ParallelRangeNode, self).__init__(pos, **kwds)
@@ -10001,6 +10023,10 @@ class ParallelRangeNode(ParallelStatNode):
             reduction_codepoint = self.parent.privatization_insertion_point
         else:
             code.put("#pragma omp parallel")
+
+            # Add if-statements to code
+            self.put_if_(code)
+
             self.privatization_insertion_point = code.insertion_point()
             reduction_codepoint = self.privatization_insertion_point
             code.putln("")
```
That definitely looks on the right track!
We'd want some test cases (which I guess you sort of have already). They'd ideally like with the other parallel tests in tests/run/parallel.pyx (or maybe one of the other similarly named file). Ideally also a short example for the documentation but that doesn't absolutely need to be in the initial PR
>I think we'd just call the argument `if_` - that sort of name is fairly
>common elsewhere in the Python world.

Sorry for the bike shedding, but requiring underscore names for keyword arguments seems unnecessarily annoying. I'm sure there's a nicer name. Maybe "only_if"? I'm not sure if that's better, but at least it doesn't look like we just failed to find a better name.

I partially agree, but the clear reference to the openmp specification is obvious when using this name. I will not object to any choice.  
But I would prefer a clear nomenclature resembling the openmp specification. 

I would also think that people trying out Cython would quite quickly realize that `if` is a reserved word, hence the _, it is not uncommon practice, take https://docs.python.org/3/library/operator.html as an example.
> I would also think that people trying out Cython would quite quickly realize that `if` is a reserved word, hence the _, it is not uncommon practice

Yes - that was largely my logic in suggesting it. Similar things seem to be the de-facto way of writing names that conflict with keywords (PyQt `exec_` was in my mind as an example too).

With that said, I don't feel too strongly and would be happy with other sensible options. If/when you have something you're close to happy with, don't delay submitting it because you're unsure of a name.

Commit message: Add `if` parameter to `parallel()` and `prange()`

Patch to review:
diff --git a/Cython/Compiler/Nodes.py b/Cython/Compiler/Nodes.py
index fc4a31f180b..c625030ccd0 100644
--- a/Cython/Compiler/Nodes.py
+++ b/Cython/Compiler/Nodes.py
@@ -9079,7 +9079,7 @@ class ParallelStatNode(StatNode, ParallelNode):
                                 construct (replaced by its compile time value)
     """
 
-    child_attrs = ['body', 'num_threads']
+    child_attrs = ['body', 'num_threads', 'if_']
 
     body = None
 
@@ -9090,6 +9090,7 @@ class ParallelStatNode(StatNode, ParallelNode):
 
     num_threads = None
     chunksize = None
+    _if = None
 
     parallel_exc = (
         Naming.parallel_exc_type,
@@ -9132,9 +9133,10 @@ def analyse_declarations(self, env):
         self.body.analyse_declarations(env)
 
         self.num_threads = None
+        self.if_ = None
 
         if self.kwargs:
-            # Try to find num_threads and chunksize keyword arguments
+            # Try to find num_threads, if_ and chunksize keyword arguments
             pairs = []
             seen = set()
             for dictitem in self.kwargs.key_value_pairs:
@@ -9144,6 +9146,9 @@ def analyse_declarations(self, env):
                 if dictitem.key.value == 'num_threads':
                     if not dictitem.value.is_none:
                         self.num_threads = dictitem.value
+                elif dictitem.key.value == 'if_':
+                    if not dictitem.value.is_none:
+                        self.if_ = dictitem.value
                 elif self.is_prange and dictitem.key.value == 'chunksize':
                     if not dictitem.value.is_none:
                         self.chunksize = dictitem.value
@@ -9170,6 +9175,12 @@ def analyse_expressions(self, env):
         if self.num_threads:
             self.num_threads = self.num_threads.analyse_expressions(env)
 
+        if self.if_:
+            if self.is_parallel:
+                self.if_ = self.if_.analyse_expressions(env)
+            else:
+                error(self.pos, "if_ must de declared in the parent parallel section")
+
         if self.chunksize:
             self.chunksize = self.chunksize.analyse_expressions(env)
 
@@ -9779,9 +9790,10 @@ class ParallelWithBlockNode(ParallelStatNode):
     This node represents a 'with cython.parallel.parallel():' block
     """
 
-    valid_keyword_arguments = ['num_threads']
+    valid_keyword_arguments = ['num_threads', 'if_']
 
     num_threads = None
+    if_ = None
 
     def analyse_declarations(self, env):
         super().analyse_declarations(env)
@@ -9793,9 +9805,17 @@ def generate_execution_code(self, code):
         self.declare_closure_privates(code)
         self.setup_parallel_control_flow_block(code)
 
+        if self.if_ is not None:
+            self.if_.generate_evaluation_code(code)
+
         code.putln("#ifdef _OPENMP")
         code.put("#pragma omp parallel ")
 
+        # Add if statement, if present
+        if self.if_ is not None:
+            code.put(" if(%s)" % self.if_.result())
+
+
         if self.privates:
             privates = [e.cname for e in self.privates
                         if not e.type.is_pyobject]
@@ -9848,7 +9868,7 @@ class ParallelRangeNode(ParallelStatNode):
     nogil = None
     schedule = None
 
-    valid_keyword_arguments = ['schedule', 'nogil', 'num_threads', 'chunksize']
+    valid_keyword_arguments = ['schedule', 'nogil', 'num_threads', 'chunksize', 'if_']
 
     def __init__(self, pos, **kwds):
         super().__init__(pos, **kwds)
@@ -10035,6 +10055,9 @@ def generate_execution_code(self, code):
 
             fmt_dict[name] = result
 
+        if self.if_ is not None:
+            self.if_.generate_evaluation_code(code)
+
         fmt_dict['i'] = code.funcstate.allocate_temp(self.index_type, False)
         fmt_dict['nsteps'] = code.funcstate.allocate_temp(self.index_type, False)
 
@@ -10099,6 +10122,11 @@ def generate_loop(self, code, fmt_dict):
             reduction_codepoint = self.parent.privatization_insertion_point
         else:
             code.put("#pragma omp parallel")
+
+            # Add if-statements to code
+            if self.if_ is not None:
+                code.put(" if(%s)" % self.if_.result())
+
             self.privatization_insertion_point = code.insertion_point()
             reduction_codepoint = self.privatization_insertion_point
             code.putln("")
diff --git a/tests/run/parallel.pyx b/tests/run/parallel.pyx
index 44a3a38f2dc..850d6ff5680 100644
--- a/tests/run/parallel.pyx
+++ b/tests/run/parallel.pyx
@@ -32,6 +32,32 @@ def test_parallel():
 
     free(buf)
 
+
+def test_parallel_if(int size):
+    """
+    >>> test_parallel_if(4)
+    >>> test_parallel_if(6)
+    """
+    cdef int maxthreads = openmp.omp_get_max_threads()
+    cdef int *buf = <int *> malloc(sizeof(int) * maxthreads)
+
+    if buf == NULL:
+        raise MemoryError
+
+    with nogil, cython.parallel.parallel(if_=size > 5):
+        buf[threadid()] = threadid()
+        # Recognise threadid() also when it's used in a function argument.
+        # See https://github.com/cython/cython/issues/3594
+        buf[forward(cython.parallel.threadid())] = forward(threadid())
+
+    for i in range(maxthreads):
+        if size > 5:
+            assert buf[i] == i
+        else:
+            assert buf[i] == 0
+
+    free(buf)
+
 cdef int get_num_threads() noexcept with gil:
     print "get_num_threads called"
     return 3


Code review comments:
diff_hunk: @@ -9090,6 +9090,7 @@ class ParallelStatNode(StatNode, ParallelNode):
 
     num_threads = None
     chunksize = None
+    _if = None
path: Cython/Compiler/Nodes.py
text: @user1:
```suggestion
    if_ = None
```

I think I prefer `if_` anyway

@author:
It's a typo thanks!

diff_hunk: @@ -9079,7 +9079,7 @@ class ParallelStatNode(StatNode, ParallelNode):
                                 construct (replaced by its compile time value)
     """
 
-    child_attrs = ['body', 'num_threads']
+    child_attrs = ['body', 'num_threads', 'if_']
path: Cython/Compiler/Nodes.py
text: @user1:
This isn't consistent with `_if` below