Page MenuHomePhabricator

DAG: Use getStoreSize when expanding dynamic vector indexing
Changes PlannedPublic

Authored by arsenm on Jun 18 2019, 12:25 PM.

Details

Summary

I'm not sure why the FIXME was there, since the ABI size of the vector
shouldn't matter for the element offset.

Fixes bug 42304.

Diff Detail

Event Timeline

arsenm created this revision.Jun 18 2019, 12:25 PM
tlively accepted this revision.Jun 18 2019, 12:45 PM

That was simple! Thank you!

This revision is now accepted and ready to land.Jun 18 2019, 12:45 PM
bjope added a subscriber: bjope.Jun 18 2019, 12:47 PM
bjope added inline comments.Jun 18 2019, 12:58 PM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
5573

Aren't vectors (of non-bytesized elements) packed when stored to memory?

I know that there is code that assumes a packed layout, but also code that does not assume a packed layout. So I don't think that LLVM is consistent.

Most of the time my impression is that vector elements with irregular sizes isn't handled well (when it comes to loads/stores/bitcast) and vectorizers try to avoid creating such vectors (in combination with load/store/bitcast). Without having looked at the details here, I figure that the extractelement is lowered into doing the extract by first storing the vector to memory and then doing a read using the dynamic index. So basically extract/insert element with a dynamic index results in store of the vector, so such operations does not work well for irregular types either.

arsenm marked an inline comment as done.Jun 18 2019, 1:03 PM
arsenm added inline comments.
lib/CodeGen/SelectionDAG/TargetLowering.cpp
5573

Any vector with elements < 8-bits that touches memory is broken. I don't think there's a consistent interpretation between targets, although I think there's a general desire to move towards treating them as packed

bjope added inline comments.Jun 18 2019, 1:27 PM
lib/CodeGen/SelectionDAG/TargetLowering.cpp
5573

So the removed assert, that kind of said that this piece of code only works correctly for byte-sized elements, should probably still be there (asserting that "EltVT.getSizeInBits() == EltVT.getStoreSizeInBits()" ).

arsenm planned changes to this revision.Jun 18 2019, 5:42 PM

I'm not sure this is actually correct