This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] NFC: Generalize emitFrameOffset to support more than byte offsets.
ClosedPublic

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

Details

Summary

Refactor emitFrameOffset to accept a StackOffset struct as its offset argument.
This method currently only supports byte offsets (MVT::i8) but will be extended
in a later patch to support scalable offsets (MVT::nxv1i8) as well.

Diff Detail

Event Timeline

sdesmalen created this revision.May 2 2019, 5:38 AM
efriedma added inline comments.May 2 2019, 11:12 AM
lib/Target/AArch64/AArch64InstrInfo.cpp
2974–2975

Are you intentionally performing the "DestReg == SrcReg && Offset == 0" both here and in emitFrameOffset?

3023

"if (ShiftSize)" is always true?

3029

The usage of "Sign" here is confusing; we always want a positive immediate for the SEH directives.

3037

We should assert here that the remaining offset is zero, to make the expected control flow clear.

3041

We should assert here that SrcReg is SP.

3074

What is Offset.isZero() supposed to be checking here?

sdesmalen updated this revision to Diff 198014.May 3 2019, 8:19 AM
sdesmalen marked 6 inline comments as done.

Added several asserts.

sdesmalen added inline comments.May 3 2019, 8:23 AM
lib/Target/AArch64/AArch64InstrInfo.cpp
2974–2975

No I wasn't, thanks for pointing out!

3023

In this patch it is always true. However, in the subsequent patch D61437: [AArch64] Static (de)allocation of SVE stack objects. I add a new opcode for SVE where ShiftSize is 0 (on line 2983).

3029

I'll remove it, and add an assert to check that Sign is always positive.

3074

For the case where the StackOffset can contain both (fixed-size) bytes, and scalable bytes (i.e. MVT::nxv1i8 for SVE where we know there are 'n x bytes', with 'n' being a runtime constant), we want to use ADDXri to emit a frame-offset if:

  • Bytes are non-zero (which would mean a meaningful "add" for the fixed-size part of the stack offset)

Or:

  • To copy the value of SrcReg to DestReg, but only if StackOffset is empty in its entirety (checking that Bytes are non-zero is not enough).

The functionality is added in D61437: [AArch64] Static (de)allocation of SVE stack objects. (lines 3067-3082).

efriedma accepted this revision.May 3 2019, 4:45 PM

LGTM

lib/Target/AArch64/AArch64InstrInfo.cpp
3074

Oh, you don't want to emit the mov if the offset isn't empty because it would be redundant. That makes sense.

This revision is now accepted and ready to land.May 3 2019, 4:45 PM
sdesmalen updated this revision to Diff 213032.Aug 2 2019, 6:23 AM
sdesmalen added a reviewer: greened.
  • Rebased patch.
sdesmalen updated this revision to Diff 213060.Aug 2 2019, 9:18 AM

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

sdesmalen updated this revision to Diff 213593.Aug 6 2019, 6:16 AM
  • Rebased patch.
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2019, 8:06 AM