[PATCH] D88591: [WebAssembly] Emulate v128.const efficiently
Hubert Tong via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Oct 2 20:42:57 PDT 2020
hubert.reinterpretcast added inline comments.
================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1631
+ Result = DAG.getSplatBuildVector(MVT::v2i64, DL,
+ DAG.getConstant(Splatted, DL, MVT::i64));
+ if (!FirstHalfSufficient && !SecondHalfSufficient && !CombinedSufficient) {
----------------
dweber wrote:
> dweber wrote:
> > dweber wrote:
> > > hubert.reinterpretcast wrote:
> > > > dweber wrote:
> > > > > hubert.reinterpretcast wrote:
> > > > > > `Splatted` represents a different value on big endian systems at this point.
> > > > > @hubert.reinterpretcast If you remove the byte_swap function all together, do you still get the same issue?
> > > > I mean, from code inspection, without needing to assume anything weird on the original `byte_swap`.
> > > >
> > > > Using half the width as an example (4 elements of 16 bits):
> > > > Values:
> > > > ```
> > > > 0x1234 0x5678 0x1234 0x5678
> > > > ```
> > > >
> > > > Resulting bytes in storage:
> > > > ```
> > > > 0x34 0x12 0x78 0x56 // 0x34 0x12 0x78 0x56
> > > > 0xff 0xff 0xff 0xff // 0xff 0xff 0xff 0xff
> > > > ```
> > > >
> > > > Request constant from value with bytes:
> > > > ```
> > > > 0x34 0x12 0x78 0x56
> > > > ```
> > > > i.e., on little endian systems, request constant with value:
> > > > ```
> > > > 0x56781234
> > > > ```
> > > > or, on big endian systems, request constant with value:
> > > > ```
> > > > 0x34127856
> > > > ```
> > > @hubert.reinterpretcast the reason I ask is because webassembly assumes little endian. On big endian machines, the constant could have been converted before we do anything here -- especially since the developer using emscripten is taught there is no other byte order but little endian. If that's the case, the byte swap could be creating the problem.
> > >
> > >
> > (the above byte_swap on a little endian machine will always produce the original parameter without fail).
> Two byte swaps would definitely be safe though.
The first `byte_swap` is necessary. The following makes the case pass:
```
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
index 8474e50..5a94041 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
@@ -1586,8 +1586,10 @@ SDValue WebAssemblyTargetLowering::LowerBUILD_VECTOR(SDValue Op,
// we can construct the 128-bit constant from a pair of i64s using a splat
// followed by at most one i64x2.replace_lane. Also keep track of the lanes
// that actually matter so we can avoid the replace_lane in more cases.
- std::array<uint64_t, 2> I64s({0, 0});
- std::array<uint64_t, 2> ConstLaneMasks({0, 0});
+ using llvm::support::ulittle64_t;
+ const ulittle64_t ulittle64_zero(0);
+ std::array<ulittle64_t, 2> I64s({ulittle64_zero, ulittle64_zero});
+ std::array<ulittle64_t, 2> ConstLaneMasks({ulittle64_zero, ulittle64_zero});
uint8_t *I64Bytes = reinterpret_cast<uint8_t *>(I64s.data());
uint8_t *MaskBytes = reinterpret_cast<uint8_t *>(ConstLaneMasks.data());
unsigned I = 0;
```
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D88591/new/
https://reviews.llvm.org/D88591
More information about the llvm-commits
mailing list