This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Fix issues with scalable-vector INSERT/EXTRACT_SUBVECTORs
ClosedPublic

Authored by frasercrmck on Mar 12 2021, 5:16 AM.

Details

Summary

This patch addresses a few issues when dealing with scalable-vector
INSERT_SUBVECTOR and EXTRACT_SUBVECTOR nodes.

When legalizing in DAGTypeLegalizer::SplitVecRes_INSERT_SUBVECTOR, we
store the low and high halves to the stack separately. The offset for
the high half was calculated incorrectly.

Additionally, we can optimize this process when we can detect that the
subvector is contained entirely within the low/high split vector type.
While this optimization is valid on scalable vectors, when performing
the 'high' optimization, the subvector must also be a scalable vector.
Note that the 'low' optimization is still conservative: it may be
possible to insert v2i32 into the low half of a split nxv1i32/nxv1i32,
but we can't guarantee it. It is always possible to insert v2i32 into
nxv2i32 or v2i32 into nxv4i32+2 as we know vscale is at least 1.

Lastly, in SelectionDAG::isSplatValue, we early-exit on the extracted subvector value
type being a scalable vector, forgetting that we can also extract a
fixed-length vector from a scalable one.

Diff Detail

Event Timeline

frasercrmck created this revision.Mar 12 2021, 5:16 AM
frasercrmck requested review of this revision.Mar 12 2021, 5:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 5:16 AM
frasercrmck retitled this revision from [CodeGen] Fix issues with scalable-vector INSERT_SUBVECTORs to [CodeGen] Fix issues with scalable-vector INSERT/EXTRACT_SUBVECTORs.Mar 12 2021, 5:21 AM
frasercrmck edited the summary of this revision. (Show Details)
RKSimon added inline comments.Mar 12 2021, 5:28 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1281

Move these back to the old location?

1291

Move this back to the old location?

  • address feedback: move some lines back to their original place
frasercrmck marked 2 inline comments as done.Mar 12 2021, 6:42 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1291

Done and done.

craig.topper added inline comments.Mar 12 2021, 10:32 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1291

Merge these with an && to decrease indentation?

frasercrmck marked an inline comment as done.
  • rebase
  • update test (frame layout changes)
  • decrease indentation
frasercrmck marked an inline comment as done.Mar 15 2021, 4:02 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1291

Yep, makes sense. Done.

david-arm added inline comments.Mar 15 2021, 8:11 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1289

Hi @frasercrmck, I'm not sure if this even works for scalable vectors? The LoVT is scalable and you need to know the full number of elements in LoVT rather than the minimum number here. Suppose vscale = 2 and LoElems = 8, then what happens when IdxVal = 9? I think it's still accessing the Lo half in this case since the number of elements in Lo is actually 16.

I suspect you may need to restrict this further to:

if (VecVT.isFixedLengthVector() && SubVecVT.isFixedLengthVector() && ...
frasercrmck marked 2 inline comments as done.Mar 15 2021, 8:19 AM
frasercrmck added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1289

Hi. For insert_/extract_subvector, when the subvector is a scalable type, then the index is implicitly multiplied by vscale. So it should be correct to use the minimum number in this case: IdxVal in your example is actually 18.

david-arm added inline comments.Mar 15 2021, 8:22 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp
1289

Ah yes of course! I'd forgotten about - thanks for pointing it out.

This revision is now accepted and ready to land.Mar 15 2021, 9:20 AM
This revision was automatically updated to reflect the committed changes.
frasercrmck marked an inline comment as done.