ENH: umath: ensure ufuncs are well-defined with memory overlapping inputs by pv · Pull Request #8043 · numpy/numpy

pv · 2016-09-11T16:51:55Z

Make ufuncs and ufunc operations to make temporary copies if input and output operands overlap in memory. Currently (cf. gh-6272, gh-1683) output from such operations is undefined. With this PR, the result is the same as if the operands were non-overlapping.

Fixes gh-1683

Adds new flags to NpyIter, and one new API function is added to deal with MapIter.

This PR does not fix up overlap handling in array assignment, that needs to be done later (and is slightly more hairy --- some common overlapping cases are in fact already handled). Unlike adding overlap-if-copy flag to NpyIter, it appears a similar flag would not be as useful in MapIter, as there are many special-cased code paths, and MapIter is not always made aware of all arrays operated on.

reread whole patch
re-check test coverage, and whether some of the tests are unnecessary

charris · 2016-11-06T16:56:24Z

numpy/core/src/private/lowlevel_strided_loops.h

Seems rather large for inline ;)

OTOH, I don't really want to put it in a .c file since the logic is tightly coupled to the macros here (and I suspect the multiarray/umath split adds to the mess). Removing the inline makes it emit compiler warnings. I could write this as a macro, but that's messy.

C++ beckons ;)

charris · 2016-11-06T16:57:28Z

numpy/core/tests/test_mem_overlap.py

Line too long ?

charris · 2016-11-06T17:03:21Z

numpy/core/code_generators/numpy_api.py

Should update this now that 1.12 is branched. The cversion and C-API number also need updating, see documentation in doc/HOWTO_RELEASE.rst.txt.

charris · 2016-11-06T17:04:12Z

Just starting to look at this.

pv · 2016-11-12T13:42:46Z

numpy/core/src/umath/ufunc_object.c

Now that I look at this again, I guess op_tmp should be placed back into op instead of being discarded.

charris · 2016-12-06T19:45:01Z

doc/source/reference/c-api.iterator.rst

?. What other array? I think the operation here could be best explained with the help of some diagrams showing the data access and writebacks. The operation can be performed, and is valid, even if there is no overlap, just that it isn't necessary if no overlap.

I think this might be help clarify behavior when the indexing is many to one in either array.

charris · 2016-12-06T19:52:41Z

doc/source/reference/c-api.iterator.rst

This is only for two operands? For inplace operations, both inputs are read, but only one is written too. Or does this apply for three operands, two inputs and one output. Or is it more general than that?

It applies to any number of operands.

charris · 2016-12-06T19:57:18Z

numpy/core/src/multiarray/mapping.c

Hmm, so you are actually checking indexes rather than the resulting view?

It's used to checking the overlap between the array operated on, and the index array used in ufunc.at, for example

x = np.arange(100) np.invert.at(x[::-1], x) # <- crazy stuff if no x.copy() of the second argument

It doesn't look at the content of the indices, so it's a somewhat conservative check.

charris · 2016-12-06T20:10:28Z

Just a first read through. This will need a release note eventually.

charris · 2016-12-06T21:06:14Z

Probably should bring in @seberg on this also.

pv · 2017-01-10T21:13:03Z

numpy/core/tests/test_mem_overlap.py

I would need to check again if these tests cover all places where the copyifoverlap behavior is in, and/or whether some of the tests are unnecessary.

ok, checked --- removing any of NPY_COPY_IF_OVERLAP et al. causes some test to fail

juliantaylor · 2017-01-12T13:20:51Z

numpy/core/src/umath/ufunc_object.c

why is this line added? a bug in the old code or due to the other changes?

I think it's a noop. Not necessary to add.

On second thought, I think it's useful to have here to ensure correctness, if we later on change NPY_ITER_COPY_IF_OVERLAP to make copies of also the input arrays.

Add a new NPY_ITER_COPY_IF_OVERLAP iterator flag to NpyIter, which instructs it to check if read operands overlap with write operands in memory, and make temporary copies to eliminate detected overlap. Thanks to Sebastian Berg.

These can differ if the iterator decides to make temporary copies of output arrays.

…ABLE loops These loops iterate over whole arrays in "trivial" order, so that it is possible to reason about the data dependency.

After this commit, all code paths in PyUFunc_GeneralizedFunction and PyUFunc_GenericFunction perform input/output operand memory overlap detection, and make copies to eliminate it.

…c_loops Because the wheremask loop does not write to all elements of the array, it needs to consider output arrays as READWRITE, in case COPY_IF_OVERLAP makes temporary copies. As the iterator may make temporary copies of arrays, make sure to pass those to __array_prepare__ instead of the originals.

… overlap

This adds a new method PyArray_MapIterArrayCopyIfOverlap to the API.

njsmith · 2017-01-23T04:12:49Z

Oh, right, now I understand! ...this rings a bell, too. @seberg, weren't you telling me at some point about how there's some code inside numpy that relies on implementing reducations via zero-strided calls to the non-reduce loops? Would that be affected by this? (My guess is that whatever code is using this trick is "below" the overlap checking in the stack so unaffected by it, and I'm totally happy with this not being something we guarantee to end-users. Also hopefully no-one is writing horrible as_strided expressions as a "shorthand" instead of calling np.sum!)

pv · 2017-01-23T08:48:41Z

The copies are triggered only if the iterator is given NPY_ITER_COPY_IF_OVERLAP flag, which I was careful to set only if I understand the code --- so it is not set for the reduction iterators, and the overlap issue is handled before the iterator.

…terArrayCopyIfOverlap

charris · 2017-01-29T03:31:50Z

@pv Do you feel that this is about ready?

pv · 2017-02-06T21:22:10Z

@charris: I've read this several times through now, and only finding forgotten decref on error handling. I'm fairly confident now that it's what it says on the tin, and doesn't do anything more.

It's a bit ugly that this adds PyArray_MapIterArrayCopyIfOverlap to the API --- it's essentially a private function, as was PyArray_MapIterArray --- but the better way is not obvious. On the other hand, adding the new iterator flags appears to be a good way to do this, as it makes reasoning about the change easier, and the changes in ufuncs themselves are mostly minimal. Currently, the copying is somewhat conservative in that perhaps some could be avoided (e.g. in accumulate) and the heuristics in deciding what to copy is a bit primitive, but that's something that in principle can be tightened up later.

pv · 2017-02-06T21:25:54Z

One additional thing is that this probably won't work on PyPy due to the use of updateifcopy, but I guess that's part of a bigger pypy issue that needs separate fixes (probably sprinkling PyArray_FlushUpdateIfCopy around) in more places than just this.

charris · 2017-02-08T23:05:37Z

@mattip @rlamy Comments?

rlamy · 2017-02-09T02:10:34Z

Well, I don't really understand the UPDATEIFCOPY issue, but since @mattip is on holiday, I'll comment anyway.

All the new tests appear to pass on both pypy and pypy3. I'm guessing that's because the copy cannot be seen by the RPython GC. So I think this is OK for us.

pv · 2017-02-09T09:30:20Z

Yes, the updateifcopy arrays here are never passed back to Python. If cpyext is effectively refcounted in this case, then there should be no problem. I guess things would break also in other places if this wasn't the case, as the iterator uses updateifcopy temporaries also for other purposes.

charris · 2017-02-10T03:32:16Z

numpy/core/src/multiarray/mapping.c

+
+    if (index_type & (HAS_FANCY | HAS_BOOL)) {
+        for (i = 0; i < num; ++i) {
+            if (indices[i].object != NULL && PyArray_Check(indices[i].object) &&


Should have extra indent here to separate the condition from the block.

Bit tricky with the long line, but it doesn't read well as is.

charris · 2017-02-10T03:33:02Z

numpy/core/src/multiarray/mapping.c

+        }
+    }
+
+    if (extra_op != NULL && PyArray_Check(extra_op) &&


charris · 2017-02-10T04:04:10Z

numpy/core/src/multiarray/mapping.c


+/*NUMPY_API
+ *
+ * Use advanced indexing to iterate an array. Please note


This needs to be documented somewhere public. It seems dangerous to expose an API that may change...

How could someone detect the change to this particular function?

This the is old comment for PyArray_MapIterArray, not added by me.

Anyway, removing it, since it's unlikely we will change this...

charris · 2017-02-10T04:07:18Z

numpy/core/src/multiarray/nditer_constr.c

    }

+    if (flags & NPY_ITER_COPY_IF_OVERLAP) {
+        /* Perform operand memory overlap checks if requested */


Could add more detail on what is being checked here, the notes below are a bit too local.

charris · 2017-02-10T04:09:15Z

numpy/core/src/multiarray/nditer_constr.c

+                 * make sense to go further.
+                 */
+                may_share_memory = solve_may_share_memory(
+                    op[iop], op[iother], 1);


Maybe extra indent here, or even break the args and align.

charris · 2017-02-10T04:14:05Z

numpy/core/src/private/lowlevel_strided_loops.h

+        return 1;
+    }
+
+    /* Arrays overlapping in memory may be equivalently iterable


Multiline comment not correctly formatted ;) Also below.

charris · 2017-02-10T04:44:56Z

numpy/core/src/private/lowlevel_strided_loops.h

+    }
+    else if (stride1 < 0) {
+        arr1_ahead = (stride1 <= stride2 &&
+                      (npy_uintp)PyArray_BYTES(arr1) <= (npy_uintp)PyArray_BYTES(arr2));


Hmm... Out of curiousity, did the compiler complain without the cast?

The casts are not needed (probably leftovers from earlier versions).

charris · 2017-02-10T04:47:45Z

numpy/core/src/umath/ufunc_object.c

    for (i = nin; i < nop; ++i) {
+        /*
+         * Because we don't write to all elements, the output arrays
+         * must be considered READWRITE by the iterator.


Updateifcopy copies all elements back, not only those for which wheremask is true, so the old content of the output array must be copied there.

charris · 2017-02-10T04:52:36Z

Not done reviewing yet. Determining when a copy isn't needed for overlapping arrays seems tricky except in single operand the linear case. ISTR that the iterator may coalesce axes.

It's unlikely we will break backward compatibility, so better not claim we will do it in the code.

charris · 2017-02-16T22:41:30Z

I'm going to put this in so it will start getting more testing. I don't claim to have understood all the details, but I doubt I will discover any errors by inspection. The write up in the release notes is very nice, you should consider copying that over to the official documentation. There are still some style nits that I may attempt to clean up later, I would gently urge you to pay more attention to spaces around operators ('+') and line length, even though the latter may not always be as readable.

Thanks for this work, Pauli.

seberg · 2017-02-17T10:51:30Z

Wow, cool. Assuming nothing goes haywire, this is the next huge addition :)!

pv mentioned this pull request Sep 11, 2016

ENH: NpyIter: add a flag to handle read/write operand overlap #8026

Closed

pv force-pushed the ufunc-copy-overlap branch from 5537a37 to 48253f2 Compare September 11, 2016 17:42

charris added 01 - Enhancement component: numpy._core labels Sep 11, 2016

pv mentioned this pull request Sep 19, 2016

BUG: "sp.linalg.solve_discrete_are" fails for random data scipy/scipy#6572

Merged

charris added this to the 1.13.0 release milestone Nov 4, 2016

charris reviewed Nov 6, 2016

View reviewed changes

numpy/core/tests/test_mem_overlap.py Outdated

Copy link

Member

charris Nov 6, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line too long ?

charris reviewed Nov 6, 2016

View reviewed changes

pv commented Nov 12, 2016

View reviewed changes

charris reviewed Dec 6, 2016

View reviewed changes

njsmith mentioned this pull request Jan 2, 2017

Numpy.dot silently returns all zero matrix when the out argument is the same as the input. #8440

Closed

pv commented Jan 10, 2017

View reviewed changes

juliantaylor reviewed Jan 12, 2017

View reviewed changes

pv added 11 commits January 19, 2017 22:12

MAINT: core: add a missing compilation dependency to setup.py

f4a5e5f

STY: core: change test docstring to comment, to prettify nose output

ed46b9e

BUG: core: fix use of wrong output array in iterator_loop

51e3a07

These can differ if the iterator decides to make temporary copies of output arrays.

ENH: core: add overlap detection logic to EQUIVALENTLY/TRIVIALLY_ITER…

e235498

…ABLE loops These loops iterate over whole arrays in "trivial" order, so that it is possible to reason about the data dependency.

ENH: umath: turn on overlap detection + copying in ufunc basic calls

7fb8e6b

After this commit, all code paths in PyUFunc_GeneralizedFunction and PyUFunc_GenericFunction perform input/output operand memory overlap detection, and make copies to eliminate it.

TST: core: add tests to check that basic ufunc calls deal with memory…

689bd47

… overlap

ENH: umath: turn on overlap detection + copying in ufunc reductions

e3f3bf7

TST: core: add tests for memory overlap in ufunc reductions

dae0b12

ENH: core: handle memory overlap in ufunc.at

0bff7b3

This adds a new method PyArray_MapIterArrayCopyIfOverlap to the API.

pv added 2 commits January 23, 2017 22:18

DOC: fix wrong flag name docstring

7098210

BUG: multiarray: fix refcount error in error handling in PyArray_MapI…

e09bd51

…terArrayCopyIfOverlap

BUG: core: fix refcount errors on error handling

da4be54

pv mentioned this pull request Feb 8, 2017

ENH: Automatic copy on overlap #6272

Closed

5 tasks

charris reviewed Feb 10, 2017

View reviewed changes

numpy/core/src/multiarray/mapping.c

}

}

if (extra_op != NULL && PyArray_Check(extra_op) &&

Copy link

Member

charris Feb 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above.

charris reviewed Feb 10, 2017

View reviewed changes

pv added 2 commits February 10, 2017 20:49

STY: core: fix up code style and add clearer comments

cf3b7e8

DOC: core: remove old comment saying public API may change

8e8ce44

It's unlikely we will break backward compatibility, so better not claim we will do it in the code.

charris merged commit d55f40b into numpy:master Feb 16, 2017

This was referenced Feb 16, 2017

ENH: Adding logical gufuncs, ie all_equal #8528

Closed

WIP: Implement oindex #6075

Closed

eric-wieser mentioned this pull request Jan 16, 2018

Invalid result from Reduction Iteration in given example #10410

Closed

grafi-tt mentioned this pull request Dec 7, 2018

Fused function copies return values cupy/cupy#1248

Open

Uh oh!

Conversation

pv commented Sep 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pv Jan 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Nov 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris Dec 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pv Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Dec 6, 2016

Uh oh!

charris commented Dec 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

njsmith commented Jan 23, 2017

Uh oh!

pv commented Jan 23, 2017 via email

Uh oh!

charris commented Jan 29, 2017

Uh oh!

pv commented Feb 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pv commented Feb 6, 2017

Uh oh!

charris commented Feb 8, 2017

Uh oh!

rlamy commented Feb 9, 2017

Uh oh!

pv commented Feb 9, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pv commented Sep 11, 2016 •

edited

Loading

pv Jan 22, 2017 •

edited

Loading

charris Dec 6, 2016 •

edited

Loading

pv Jan 10, 2017 •

edited

Loading

pv commented Feb 6, 2017 •

edited

Loading

charris commented Feb 16, 2017 •

edited

Loading