Page MenuHomePhabricator

Add OffsetIsScalable to getMemOperandWithOffset
ClosedPublic

Authored by sdesmalen on Jan 15 2020, 3:44 AM.

Details

Summary

Making Scale a TypeSize in AArch64InstrInfo::getMemOpInfo,
has the effect that all places where this information is used
(notably, TargetInstrInfo::getMemOperandWithOffset) will need
to consider Scale - and derived, Offset - possibly being scalable.

This patch adds a new operand bool &OffsetIsScalable to
TargetInstrInfo::getMemOperandWithOffset and fixes up all
the places where this function is used, to consider the
offset possibly being scalable.

In most cases, this means bailing out because the algorithm does not
(or cannot) support scalable offsets in places where it does some
form of alias checking for example.

Diff Detail

Event Timeline

sdesmalen created this revision.Jan 15 2020, 3:44 AM
efriedma added inline comments.Jan 15 2020, 3:15 PM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1254

I'm not sure why this modification is necessary. The offset doesn't actually scale, does it? The width of memory operation does, but getMemOperandWithOffset doesn't return that anyway.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
964

Is computing overlap on scalable vectors here really sound?

llvm/lib/Target/Lanai/LanaiInstrInfo.h
79

Do we need to change getMemOperandWithOffsetWidth now? It's a target-specific method.

sdesmalen marked 3 inline comments as done.Jan 21 2020, 11:13 AM
sdesmalen added inline comments.
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1254

For e.g. AArch64::LDR_ZXI and AArch64::STR_ZXI, it is actually the offset that is scaled by VL elements (i.e. load/store at ptr + (N * sizeof(vector)).

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
964

I think so; it assumes that the runtime scaling of OffsetA is the same as that of OffsetB. With runtime scaling being vscale for both vectors and the width being the worst-case width (256 bytes), I think we can determine whether there is no overlap. The function seems otherwise conservative and assumes overlap.

llvm/lib/Target/Lanai/LanaiInstrInfo.h
79

Good point, that indeed seems unnecessary. I'll remove those changes.

efriedma added inline comments.Jan 21 2020, 1:01 PM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1254

Oh, that makes sense.

I guess there are theoretically two separate dimensions: whether the width is scalable, and whether the offset is scalable. For SVE, I guess those happen to be the same for all the defined instructions?

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
964

Oh, I didn't understand the part about the width; maybe worth adding a comment to the declaration of getMemOperandWithOffsetWidth.

sdesmalen updated this revision to Diff 244983.Mon, Feb 17, 8:37 AM
sdesmalen marked 4 inline comments as done.
  • Removed unnecessary interface changes from getMemOperandWithOffsetWidth for other targets than AArch64.
  • Added comment to AArch64InstrInfo::getMemOperandWithOffsetWidth.
sdesmalen marked 2 inline comments as done.Mon, Feb 17, 8:39 AM
sdesmalen added inline comments.
llvm/include/llvm/CodeGen/TargetInstrInfo.h
1254

Yes, that's correct.

This revision is now accepted and ready to land.Mon, Feb 17, 12:15 PM
This revision was automatically updated to reflect the committed changes.