Page MenuHomePhabricator

[CodeGen,AArch64] Fix up warnings in splitStores
ClosedPublic

Authored by david-arm on May 28 2020, 6:29 AM.

Details

Summary

The code for trying to split up stores is designed for NEON vectors,
where we support arbitrary alignments. It's an optimisation designed
to improve performance by using smaller, aligned stores. However,
we currently only support 16 byte alignments for SVE vectors anyway
so we may as well bail out early.

This change fixes up remaining warnings in a couple of tests:

CodeGen/AArch64/sve-callbyref-notailcall.ll
CodeGen/AArch64/sve-calling-convention-byref.ll

Diff Detail

Event Timeline

david-arm created this revision.May 28 2020, 6:29 AM
david-arm updated this revision to Diff 268067.Jun 2 2020, 11:57 PM
david-arm edited the summary of this revision. (Show Details)
sdesmalen added inline comments.Jun 3 2020, 12:59 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12142

VT.isScalableVector() ?

12144

It's a bit odd to assert !VT.isScalableVector() here given the condition above, but I also don't really see why you're asserting the alignment here in this function. What was the motivation for adding it?

david-arm marked 3 inline comments as done.Jun 3 2020, 11:19 PM
david-arm added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12142

The original code bailed out if it wasn't a vector, now we want to bail if it's not a fixed length vector. That's why I can't just use

if (VT.isScalableVector())

as it would then allow non-vector types in.

12144

So we can get here for two cases:

  1. It's not a vector at all, hence the assert(!VT.isScalableVector() || ..)
  2. It's a scalable vector. This function has been specifically written to optimise cases of NEON vector stores to addresses with terrible alignments, i.e. not 16 bytes. It's hard to split up stores for SVE, although not impossible - and the cost of doing so probably makes it not worth it. However, we shouldn't even have to worry about mis-alignments for SVE vectors at all because they are always supposed to be aligned to 16 bytes. That's why I put the assert in, although I admit it may be unnecessary. I can remove it if you want?
sdesmalen added inline comments.Mon, Jun 8, 5:14 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
12142

Okay that makes sense, thanks for clarifying!

12144

We indeed require an alignment of 16 for scalable vectors allocated on the stack, it's more that given that this code is not doing anything with scalable vectors other than bailing out, having the assert here seems unnecessary. There are already asserts in FrameLowering to cover this.

david-arm updated this revision to Diff 269193.Mon, Jun 8, 6:01 AM
david-arm marked an inline comment as done.
david-arm marked 3 inline comments as done.
This revision is now accepted and ready to land.Mon, Jun 8, 10:01 AM
This revision was automatically updated to reflect the committed changes.