This is an archive of the discontinued LLVM Phabricator instance.

AArch64: use ldp/stp for 128-bit atomic load/store with v8.4
ClosedPublic

Authored by t.p.northover on Sep 15 2021, 7:04 AM.

Details

Reviewers
paquette
efriedma
Summary

Prior to v8.4a[*] the only way to get a 128-bit atomic load or store was via ldxp/stxp (or casp), which is not only inefficient but outright impossible without write access. So v8.4a extended the memory model so that any 16-byte operation aligned to 16 bytes (as all LLVM atomic load/stores must be) is atomic.

This patch implements ISel for these instructions in both SDAG and GISel. In both cases we go for ldp/stp implementations since atomics are much more likely to be GPR-based operations.

Diff Detail

Event Timeline

t.p.northover created this revision.Sep 15 2021, 7:04 AM
t.p.northover requested review of this revision.Sep 15 2021, 7:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 7:04 AM

Added separate feature to control it (named from ARMARM), since it's optional from v8.2 onwards.

paquette added inline comments.Sep 15 2021, 10:52 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
17643

might as well make this an if?

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
1031

I think mi_match could really simplify this function.

if (mi_match(Root, MRI, m_GPtrAdd(m_Reg(NewBase), m_ICst(NewOffset))) && isShiftedInt<7, 3>(NewOffset))) {
  Base = NewBase;
  Offset = NewOffset;
}

... But I'm not sure about the >64 bit widths.

Missing tests?

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
922

Why not custom legalize? Not that it really matters, I guess.

16341

Assert that we have the expected atomic ordering?

16345

Would it make sense to add an AArch64ISD node to use here, instead of doing instruction selection early?

17422

What are you trying to accomplish here?

t.p.northover marked 4 inline comments as done.Sep 16 2021, 4:59 AM
t.p.northover added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
922

It gets converted to a cmpxchg during LegalizeTypes, and I don't think we can hook in there.

16345

Good idea. That should let us use SelectAddrMode... from ISelDAGToDAG instead of reinventing it here.

17422

Ah, I've got a separate patch to use CASP if LSE is present but not this LSE2. They started off together and I think this is a remnant of that bit. I'll remove it.

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp
1031

Oh, neat! Didn't know that existed.

t.p.northover marked 2 inline comments as done.Sep 16 2021, 5:12 AM
t.p.northover added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
16345

Actually, it looks like someone has beaten me to it with volatile load/store, and I was wrong about not being able to hook into LegalizeTypes. I should be able to reuse most of their work.

  • Added tests.
  • Switched to existing SDAG LDP/STP route for volatiles.
  • Simplified GISel path with matching expressions.
  • Think I fixed everything else mentioned.
This revision is now accepted and ready to land.Sep 16 2021, 11:39 AM
t.p.northover closed this revision.Sep 20 2021, 1:50 AM

Thanks. Committed as 13aa102e0769.