[PATCH] D88591: [WebAssembly] Emulate v128.const efficiently

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1603
+        auto *Const = cast<ConstantSDNode>(Lane.getNode());
+        uint64_t Val = byte_swap(Const->getLimitedValue(), little);
+        uint8_t *ValPtr = reinterpret_cast<uint8_t *>(&Val);
----------------
dweber wrote:
> efriedma wrote:
> > dweber wrote:
> > > efriedma wrote:
> > > > I think this byte_swap isn't doing the right thing; it's in the wrong width.
> > > > 
> > > > There are a couple of reasonable ways to write this:
> > > > 
> > > > 1. Construct an `std::array<uint8_t, 16>/std::array<ulittle16_t, 8>/std::array<ulittle32_t, 4>`, and memcpy from it to an `std::array<ulittle64_t, 2>`.
> > > > 2. Don't type-pun at all; something like:
> > > > 
> > > > 
> > > >     for (int i = 0; i < NumElements; ++i)
> > > >       Result[i / (NumElements / 2)] |= Elements[i] << ElementWidth * (i  % (NumElements / 2);
> > > When I wrote this, I checked the implementation of byte swap. It's a template that derives the byte_swap from the value type. In this case, getLimitedValue returns u64. It should be right. It makes me wonder since webassembly is guaranteed to be little endian if a conversion occurs before this happens.
> > Oh, you're extending to u64, putting a little-endian u64 into memory, and copying the first N bits.  That's effectively the same as putting a little-endian uN into memory.  But I guess that leaves you with a problem at the other end: the elements of I64s are never swapped back from little-endian to native endianness.
> > 
> > In any case, the interaction between the endianness and the pointer manipulation is hard to follow; I'd suggest rewriting it even if the current implementation were correct.
> They're not supposed to be converted back to native endianness unless there's something I misunderstand about the implementation. If two byte swaps are required, I'm pretty sure none is required.
I'm working on a rewrite that uses @efriedma's suggestion to not use type punning above, by the way.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88591/new/

https://reviews.llvm.org/D88591