This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering][AArch64][SVE] Take into account accessed type when clamping address
ClosedPublic

Authored by bsmith on Jun 28 2021, 5:48 AM.

Details

Summary

When clamping the index for a memory access to a stacked vector we must
take into account the entire type being accessed, not just assume that
we are accessing only a single element.

Diff Detail

Event Timeline

bsmith created this revision.Jun 28 2021, 5:48 AM
bsmith requested review of this revision.Jun 28 2021, 5:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 28 2021, 5:48 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7796

Question: Shouldn't this remain <? My thinking: Logically, the thing on the left is the maximum index, and when you increment that thing, it needs to remain less than NElts to still be a valid index.

7819

Did you entertain constructing a 1-element vector here and getting rid of the special case?

bsmith added inline comments.Jun 28 2021, 7:04 AM
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7796

Not quite, if NumSubElts is 1, then the index is IdxCst->getZExtValue(), not IdxCst->getZExtValue() + 1. Perhaps it's better to think of this like the following:

IdxCst->getZExtValue() + (NumSubElts - 1) < NElts

bsmith updated this revision to Diff 354890.Jun 28 2021, 7:16 AM
bsmith marked 2 inline comments as done.
  • Rearrange if statement to clarify intent
  • Construct 1-element vector in getVectorElementPointer to avoid special cases
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
7796

This looks better, thanks for clarifying.

Perhaps the comment on the if statement could be updated to account for the fact an n-element subvec is involved.

peterwaller-arm accepted this revision.Jun 28 2021, 8:17 AM
This revision is now accepted and ready to land.Jun 28 2021, 8:17 AM
This revision was landed with ongoing or failed builds.Jun 30 2021, 5:30 AM
This revision was automatically updated to reflect the committed changes.