Skip to content

Remove buggy deinterleave misfeature#5844

Merged
abadams merged 1 commit intomasterfrom
abadams/fix_deinterleave_bug
Mar 24, 2021
Merged

Remove buggy deinterleave misfeature#5844
abadams merged 1 commit intomasterfrom
abadams/fix_deinterleave_bug

Conversation

@abadams
Copy link
Member

@abadams abadams commented Mar 24, 2021

The deinterleave stores pass has some logic that generated loads from garbage addresses. It tries to recognize this:

A[ramp(0, 1, 8)] = B[ramp(0, 8, 8)]
A[ramp(8, 1, 8)] = B[ramp(1, 8, 8)]
A[ramp(16, 1, 8)] = B[ramp(2, 8, 8)]
...

and turn it into a single interleaving store. However not enough checking of the base of the ramps is done. The left-hand-side index just gets moved to the right hand side. So it triggers on something like:

A[ramp(foo, 1, 8)] = B[ramp(0, 8, 8)]
A[ramp(foo + 8, 1, 8)] = B[ramp(1, 8, 8)]
A[ramp(foo + 16, 1, 8)] = B[ramp(2, 8, 8)]
...

and then moves "foo" over into the loads. This is totally wrong and was causing crashes for me.

I just deleted this feature. That schedule can be better expressed by switching which vars are unrolled vs vectorized. It's amazing it took us 7 years to find this bug (the age of the logic in question).

@abadams abadams requested a review from dsharletg March 24, 2021 00:12
@dsharletg
Copy link
Contributor

I'm not sure we should remove this, I often do trial and error on which variable should be vectorized vs. unrolled, and we had test coverage for this? How did those tests work?

I do think we should make sure .vectorize(x).vectorize(y) generates good code in this case though, and maybe we could deprecate the vectorize/unroll approach.

@abadams
Copy link
Member Author

abadams commented Mar 24, 2021

The tests worked by hitting exactly the case that wasn't broken. In general most uses of this were probably broken. Fortunately it rarely triggered.

I looked into what checks would be necessary to make it correct, and it was much more involved than the existing code, so I just nuked the feature.

@abadams
Copy link
Member Author

abadams commented Mar 24, 2021

To be clear, the way this usually triggers is:

A[ramp(foo, 8, 8)] = x0
A[ramp(foo + 1, 8, 8)] = x1
A[ramp(foo + 2, 8, 8)] = x2
...

becomes

A[ramp(foo, 1, 64)] = interleave_vectors(x0, x1, x2, ...)

That's probably what you were hitting with trial and error, and that case is unchanged in this PR.

@steven-johnson
Copy link
Contributor

The OSX failure is unrelated (will be fixed by #5841), should be good to land

@abadams abadams merged commit 9a8ddf7 into master Mar 24, 2021
@alexreinking alexreinking added this to the v12.0.0 milestone May 19, 2021
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.

4 participants