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

[Dan Weber via Phabricator via llvm-commits]

dweber added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1600
+        I64s[I / HalfLanes] |= Val << Shift;
+        ConstLaneMasks[I / HalfLanes] |= ((1ULL << LaneBits) - 1) << Shift;
+      }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88773