This is an archive of the discontinued LLVM Phabricator instance.

[SveEmitter] Add builtins for svreinterpret
ClosedPublic

Authored by sdesmalen on Apr 23 2020, 1:47 PM.

Details

Summary

The reinterpret builtins are generated separately because they
need the cross product of all types, 121 functions in total,
which is inconvenient to specify in the arm_sve.td file.

Diff Detail

Event Timeline

sdesmalen created this revision.Apr 23 2020, 1:47 PM
Herald added a project: Restricted Project. · View Herald Transcript
efriedma added inline comments.Apr 28 2020, 6:48 PM
clang/lib/CodeGen/CGBuiltin.cpp
7925

I'm vaguely suspicious this might be wrong for big-endian targets. I mean, this isn't unreasonable, but users might be surprised if svreinterpret isn't a no-op.

sdesmalen marked an inline comment as done.Apr 29 2020, 5:53 AM
sdesmalen added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
7925

For SVE the loads and stores (svld1 and svst1) are all endian safe, so no special consideration needs to be taken for big endian targets.

The ACLE specifies that:

The svreinterpret functions simply reinterpret a vector of one type as a vector of another type, without changing any of the bits.

efriedma added inline comments.Apr 29 2020, 11:13 AM
clang/lib/CodeGen/CGBuiltin.cpp
7925

"bitcast" is specified to mean "reinterpret the bits like a store+load". On big-endian NEON (and, I assume, SVE), that isn't a no-op. See http://llvm.org/docs/BigEndianNEON.html .

I mean, if the answer here is "yes, svreinterpret is supposed to lower to a REV", then that's fine. But I'd like to see some explciit acknowledgement that that's intentional.

sdesmalen marked an inline comment as done.Apr 29 2020, 1:48 PM
sdesmalen added inline comments.
clang/lib/CodeGen/CGBuiltin.cpp
7925

Thanks for pointing out that page, but for SVE I don't think the svreinterpret should lower to a REV.

This is probably where things are different from Neon. The ACLE SVE vectors such as svint32_t are opaque vector types and the only way to load/store them from/to memory is through the use of the svld1 and svst1 intrinsics which are endian safe (in that they use the ld1/st1 instructions that do endianess conversion on big endian targets). The ACLE does not expose any full-vector load/store (ldr/str) operations.

efriedma added inline comments.Apr 29 2020, 2:20 PM
clang/lib/CodeGen/CGBuiltin.cpp
7925

Like that page describes, we use ld1/st1 for big-endian NEON, to match the LLVM IR rules for laying out a vector. If you use ld1/st1 to load/store vectors on big-endian NEON, a bitcast is not a no-op. As far as I know, SVE ld1/st1 is equivalent to NEON ld1/st1 in the case where vscale=1. Therefore, on big-endian SVE, a bitcast is not a no-op.

That leaves the following options:

  1. svreinterpret is not a no-op.
  2. svreinterpret is not equivalent to an LLVM IR bitcast, so this patch needs to be changed.
efriedma added inline comments.May 4 2020, 11:21 AM
clang/lib/CodeGen/CGBuiltin.cpp
7925

(If you don't care about big-endian SVE right now, that's fine, but please at least leave a FIXME.)

sdesmalen updated this revision to Diff 261892.May 4 2020, 12:14 PM
  • Added FIXME in CGBuiltins for big-endian svreinterpret.
  • Added diagnostic in arm_sve.h that big-endian is not yet supported.
This revision is now accepted and ready to land.May 4 2020, 12:24 PM
This revision was automatically updated to reflect the committed changes.