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

Thomas Lively via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Oct 9 17:37:38 PDT 2020


tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1600
+        I64s[I / HalfLanes] |= Val << Shift;
+        ConstLaneMasks[I / HalfLanes] |= ((1ULL << LaneBits) - 1) << Shift;
+      }
----------------
efriedma wrote:
> tlively wrote:
> > 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
> I think you missed the part where it says "if the value of the right operand is negative or is greater or equal to the number of bits in the promoted left operand, the behavior is undefined".
Ugh, yep. Thank you! Switched to using `maskTrailingOnes`.


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