efriedma added inline comments.
================ Comment at: clang/lib/CodeGen/CGBuiltin.cpp:7880 + return Builder.CreateBitCast(Val, Ty); + } + ---------------- efriedma wrote: > sdesmalen wrote: > > efriedma wrote: > > > sdesmalen wrote: > > > > efriedma wrote: > > > > > 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. > > > > 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. > > > "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. > > 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. > 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. (If you don't care about big-endian SVE right now, that's fine, but please at least leave a FIXME.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D78756/new/ https://reviews.llvm.org/D78756 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
