Page MenuHomePhabricator

[AArch64] NFC: Add generic StackOffset to describe scalable offsets.
ClosedPublic

Authored by sdesmalen on May 2 2019, 5:38 AM.

Details

Summary

To support spilling/filling of scalable vectors we need a more generic
representation of a stack offset than simply 'int'.

For this we introduce the StackOffset struct, which comprises multiple
offsets sized by their respective MVTs. Byte-offsets will thus be a simple
tuple such as { offset, MVT::i8 }. Adding two byte-offsets will result in a
byte offset { offsetA + offsetB, MVT::i8 }. When two offsets have different
types, we can canonicalise them to use the same MVT, as long as their
runtime sizes are guaranteed to have the same size-ratio as they would have
at compile-time.

When we have both scalable- and fixed-size objects on the stack, we can
create an offset that is:

({ offset_fixed, MVT::i8 } + { offset_scalable, MVT::nxv1i8 })

The struct also contains a getForFrameOffset() method that is specific to
AArch64 and decomposes the frame-offset to be used directly in instructions
that operate on the stack or index into the stack.

Note: This patch adds StackOffset as an AArch64-only concept, but we would
like to make this a generic concept/struct that is supported by all
interfaces that take or return stack offsets (currently as 'int'). Since
that would be a bigger change that is currently pending on D32530 landing,
we thought it makes sense to first show/prove the concept in the AArch64
target before proposing to roll this out further.

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.May 2 2019, 5:38 AM
rovka added a comment.May 3 2019, 4:05 AM

I'd also add a unit test for StackOffset.

lib/Target/AArch64/AArch64StackOffset.h
21 ↗(On Diff #197752)

Nitpick: This would look better as a class comment. For the file comment you could write something more brief, e.g. "\file This file contains the declaration of the StackOffset class, which is used to describe scalable and non-scalable offsets during frame lowering."

83 ↗(On Diff #197752)

Typo: simply

sdesmalen updated this revision to Diff 198423.May 7 2019, 5:04 AM
sdesmalen marked 2 inline comments as done.
  • Added unit tests for StackOffset.
  • Moved around file comments in AArch64StackOffset.h and fixed typo.

I'd also add a unit test for StackOffset.

Good point! I've added unit tests to this patch and also to D61437: [AArch64] Static (de)allocation of SVE stack objects. (which extends StackOffset for scalable vectors).

rovka accepted this revision.May 8 2019, 2:19 AM

LGTM with nits. Thanks!

unittests/Target/AArch64/TestStackOffset.cpp
27 ↗(On Diff #198423)

Nitpick: Should also test offsets different than 1.

51 ↗(On Diff #198423)

Nitpick: A negative test would also be nice.

This revision is now accepted and ready to land.May 8 2019, 2:19 AM

This was accepted. Did it land?

sdesmalen updated this revision to Diff 213030.Fri, Aug 2, 6:23 AM
sdesmalen added a reviewer: greened.
  • Rebased the patch.
  • Removed StackOffset::isZero() in favour of using StackOffset::operator bool().

This was accepted. Did it land?

Not yet, I initially thought about landing it together with D61437 because there was no need for it otherwise. But if everyone is happy, I guess this one can just land.

greened accepted this revision.Fri, Aug 2, 8:56 AM

This was accepted. Did it land?

Not yet, I initially thought about landing it together with D61437 because there was no need for it otherwise. But if everyone is happy, I guess this one can just land.

I like this as a NFC patch, so I say go ahead and land it.

sdesmalen updated this revision to Diff 213061.Fri, Aug 2, 9:19 AM

My previous patch didn't have any context, so added it now (git format-patch -U999999)

This was accepted. Did it land?

Not yet, I initially thought about landing it together with D61437 because there was no need for it otherwise. But if everyone is happy, I guess this one can just land.

I like this as a NFC patch, so I say go ahead and land it.

Thanks! I'll wait one more day to see if anyone has any objections and otherwise I'll land the patch!

sdesmalen updated this revision to Diff 213576.Tue, Aug 6, 5:01 AM
  • Rebased patch.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 6, 6:06 AM