[PATCH] D57481: [WebAssembly] Fix a regression selecting negative build_vector lanes

[Thomas Lively via Phabricator via llvm-commits]

tlively added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td:34
-  "return ((uint64_t)Imm & ((1UL << "#SIZE#") - 1)) == (uint64_t)Imm;"
->;
 foreach SIZE = [2, 4, 8, 16, 32] in
----------------
aheejin wrote:
> tlively wrote:
> > aheejin wrote:
> > > Is this unnecessary or incorrect because it assumes an unsigned integer? Looks like there are ways to specify signed immediate too: [[ https://github.com/llvm/llvm-project/blob/8a3c5b48df9dfdb4bb63c76005c58b4b1925b47e/llvm/lib/Target/X86/X86InstrInfo.td#L978-L981 | example1 ]] [[ https://github.com/llvm/llvm-project/blob/8a3c5b48df9dfdb4bb63c76005c58b4b1925b47e/llvm/lib/Target/AArch64/AArch64InstrFormats.td#L302 | example2 ]]
> > It's not incorrect, per se, but the alternative fix would be to change the custom  build_vector lowering logic to mask all the build_vector operands it produces to make them unsigned, matching the expectation here. It seemed simpler to save a few cycles and just remove this predicate.
> Sorry I don't understand. What I meant was to change the definition of `ImmI8` and `ImmI16` from unsigned to signed immediates. In that case why do we need to make operands unsigned?
Oh I understand. I like that idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D57481