Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix several bugs on Hexagon and some cleanup #5570

Merged
merged 2 commits into from Dec 21, 2020
Merged

Conversation

dsharletg
Copy link
Contributor

This PR fixes bugs:

  • vmpa is not correctly interleaved after refactoring in VectorReduce peephole matching for Hexagon #5424. This PR reverts the changes associated with this. This does have test coverage in simd_op_check, but we aren't running apps/simd_op_check on the build bots currently.
  • LetStmt visitor in EliminateInterleaves is incorrect. This has been a bug for a long time, it only appears if there are dead lets in the IR, which doesn't happen in the default lowering configuration (but does happen when attempting to debug by commenting off some of the passes).

It also removes VtmpyGenerator. This has been disabled for a long time, and now we have VectorReduce. Additionally, I have a WIP pass in another branch that finds vector reductions in a target independent lowering pass. If we try to bring back automatic finding of stencil vector reductions, we should build it into that pass instead of fixing VtmpyGenerator.

@dsharletg
Copy link
Contributor Author

@aankit-ca @pranavb-ca please take a look.

@steven-johnson
Copy link
Contributor

Windows failure is unrelated

@aankit-ca
Copy link
Contributor

@dsharletg For vmpa, I think we can simply change a01 to
Shuffle::make_interleave({mpys[0].first, mpys[1].first});
to make it work. This we way we can avoid adding the patterns in hvx_128.ll file.

@aankit-ca
Copy link
Contributor

Rest LGTM

@dsharletg
Copy link
Contributor Author

dsharletg commented Dec 21, 2020

I think we need to do the interleaving in hvx_128.ll, because the interleaving is different. Shuffle::make_interleave interleaves the vectors before splitting to native vector widths, while this implementation interleaves them after splitting to native vector widths. What do you think?

@aankit-ca
Copy link
Contributor

aankit-ca commented Dec 21, 2020

I'm sorry. I meant changing a01 to Shuffle::make_concat({mpys[0].first, mpys[1].first}); for vmpa
We will be calling make_interleave only to generate the scalar.

@dsharletg
Copy link
Contributor Author

But I think the same problem applies. By doing Shuffle::make_concat, we concatenate the vectors before slicing to native vector widths. But we need to concatenate the two operands after slicing to native vector widths, which is what the runtime hvx_128.ll implementation does.

@aankit-ca
Copy link
Contributor

Aah. Right. Yeah we will need to the concatenation in .ll

@dsharletg
Copy link
Contributor Author

Cool, thanks for confirming :) This is a tricky issue, I've had this bug before myself.

@dsharletg dsharletg merged commit 5ac8808 into master Dec 21, 2020
@dsharletg dsharletg deleted the dsharletg/fix-hexagon branch December 21, 2020 21:54
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.

None yet

3 participants