[PATCH] D88773: Reland "[WebAssembly] Emulate v128.const efficiently""

Thomas Lively via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Oct 5 12:46:28 PDT 2020


tlively added a comment.

Thanks for your continued work on this @dweber! I still think this solution is simpler than the type punning solution, though,  because it saves readers from having to go figure out what `ulittle64_t` is and how it works.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1584
     };
-  }
-  if (!Result) {
+  } else if (NumConstantLanes >= NumSplatLanes && VecT.isInteger()) {
+    // Otherwise, if this is an integer vector, pack the lane values together so
----------------
dweber wrote:
> dweber wrote:
> > @tlively this is a real edge case, but I think you need to verify that the integer type is a representable native integer type. If APInt causes VecT.isInteger() to return true, you'll have a colossal headache with supporting integers that are 27 bits. 
> 
This operation lowering occurs only after type legalization, so we know that the only machine value types we need to consider are those that WebAssembly supports. I also don't think this suggested change changes the semantics. Am I missing something?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1589-1590
+    // 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}});
+    size_t LaneBits = 128 / Lanes;
----------------
dweber wrote:
> hubert.reinterpretcast wrote:
> > tlively wrote:
> > > This is the only part that has changed from the previous revision.
> > MSVC doesn't work with the parens even with the extra braces.
> Right. It shouldn't have parens.
Thanks!


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1600
+        I64s[I / HalfLanes] |= Val << Shift;
+        ConstLaneMasks[I / HalfLanes] |= ((1ULL << LaneBits) - 1) << Shift;
+      }
----------------
dweber wrote:
> hubert.reinterpretcast wrote:
> > tlively wrote:
> > > hubert.reinterpretcast wrote:
> > > > Can `LaneBits` be 64?
> > > Yes, if the vector is already an v2i64.
> > Okay, I suggest using `maskTrailingOnes<uint64_t>(LaneBits)` to avoid undefined behaviour:
> > ```
> > diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
> > index ec62f2a..b2913b6 100644
> > --- a/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
> > +++ b/llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp
> > @@ -30,8 +30,8 @@
> >  #include "llvm/IR/Intrinsics.h"
> >  #include "llvm/IR/IntrinsicsWebAssembly.h"
> >  #include "llvm/Support/Debug.h"
> > -#include "llvm/Support/Endian.h"
> >  #include "llvm/Support/ErrorHandling.h"
> > +#include "llvm/Support/MathExtras.h"
> >  #include "llvm/Support/raw_ostream.h"
> >  #include "llvm/Target/TargetOptions.h"
> >  using namespace llvm;
> > @@ -1597,7 +1597,8 @@ SDValue WebAssemblyTargetLowering::LowerBUILD_VECTOR(SDValue Op,
> >          auto Shift = LaneBits * (I % HalfLanes);
> >          auto Val = cast<ConstantSDNode>(Lane.getNode())->getZExtValue();
> >          I64s[I / HalfLanes] |= Val << Shift;
> > -        ConstLaneMasks[I / HalfLanes] |= ((1ULL << LaneBits) - 1) << Shift;
> > +        ConstLaneMasks[I / HalfLanes] |= maskTrailingOnes<uint64_t>(LaneBits)
> > +                                         << Shift;
> >        }
> >      }
> >      // Check whether all constant lanes in the second half of the vector are
> > ```
> @tlively I think you need to add test cases for each integer type with all of the variants of 0xdeadbeef. This logic has so many branches, it would have been easier to read if it merely used if statements to compare integer types for the appropriate shifts.
Is there UB here? `1ULL << 64` is well defined to be 0ULL [1], and 0ULL - 1 is well defined to be the max int for that type [2]. The reason I'm pushing back here is that I want to save readers of this code from having to go look up what exactly `maskTrailingOnes` does.

[1] https://en.cppreference.com/w/cpp/language/operator_arithmetic#Bitwise_shift_operators:~:text=For%20unsigned%20a%2C%20the%20value%20of%20a,of%20the%20destination%20type%20are%20discarded).

[2] https://en.cppreference.com/w/cpp/language/operator_arithmetic#Overflows:~:text=Unsigned%20integer%20arithmetic%20is%20always%20performed%20modulo%202n


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88773



More information about the llvm-commits mailing list