Skip to content

Conversation

bluss
Copy link
Member

@bluss bluss commented Apr 8, 2021

This change means that we are not drawn to prefer an axis with length 1 as the "innermost/tightest" axis in Zip.

This fixes the bad performance reported in #932 (comment), specifically select_axis1. The runtime for that benchmark decreases from 21 to 3.4 μs, which is quite substantial and just due to misidentification (in that case) of which axis to prefer for the loop.

bluss added 2 commits April 8, 2021 22:22
These tests - w.r.t C/F preference, failed when they were introduced.
The issue was that otherwise, this array:
shape: [1, 256]
strides: [1, 256]

Which was "correctly" determined to be F-preference because it's
sliced out of an f-order array. In reality it is discontiguous; every
element is 256 steps from the next. This should have layout None, the
same as this array would have (ignoring 1-len axes).

shape: [256]
strides: [256]
@bluss bluss changed the title Fix Require len > 1 axis for layout preference Apr 8, 2021
@bluss bluss merged commit eb0dfa8 into master Apr 8, 2021
@bluss bluss deleted the cf-preference branch April 8, 2021 20:34
@bluss bluss added this to the 0.15.2 milestone Apr 8, 2021
@bluss
Copy link
Member Author

bluss commented Apr 8, 2021

This commit (on a pr branch - don't know if the commit will disappear) is relevant to the same problem 4878d1b (Turns out I fixed an assertion for this scenario). After the present PR was merged, the situation described in this message has changed, we fixed it to remove the "leaning f" judgment. Still, the array could be "contiguous with stride 2."

 FIX: Fix iteration order debug assertion in Zip

We're trying to assert the iteration order is like C-contig or
C-preferred. The old assertion tripped when combining these two arrays:

- shape 1, 7 strides 7, 1  (layout: CFcf)
- shape 1, 7 strides 1, 2  (layout: f)

The result was that the first was judged to have "no preference" and
second "leaning f", total "leaning f".

The debug assertion tripped a false positive because the traversal is
one-dimensional, so elements are still visited in the required order.

The layout judgment for the second array should ideally be "CFcf" too,
but then it needs an understanding of being one-dimensional with
contiguous stride of 2, which is not implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant