This is an archive of the discontinued LLVM Phabricator instance.

Differential D155386

[WebAssembly] Select BUILD_VECTOR with large unsigned lane values
AcceptedPublic

Authored by tlively on Jul 15 2023, 9:29 PM.

Details

Reviewers
aheejin
dschuff
Summary

Previously we expected lane constants to be in the range of signed values for
each lane size, but the included test case produced large unsigned values that
fall outside that range. Allow instruction selection to proceed in this case
rather than failing.

Fixes #63817.

Diff Detail

Unit TestsFailed

TimeTest
60,060 msx64 debian > MLIR.Examples/standalone::test.toy
60,080 msx64 debian > ThreadSanitizer-x86_64.ThreadSanitizer-x86_64::restore_stack.cpp

Event Timeline

tlively created this revision.Jul 15 2023, 9:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2023, 9:29 PM
Herald added subscribers: pmatos, asb, wingo and 4 others. · View Herald Transcript
tlively requested review of this revision.Jul 15 2023, 9:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 15 2023, 9:29 PM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B245643: Diff 540754.Jul 15 2023, 10:57 PM
dschuff accepted this revision.Jul 17 2023, 9:22 AM
This revision is now accepted and ready to land.Jul 17 2023, 9:22 AM
aheejin added inline comments.Jul 17 2023, 12:50 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
53

Why are the negative range and the positive range different?

tlively added inline comments.Jul 18 2023, 7:26 AM
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
53

The signed values range from -2^(n-1) to 2^(n-1)-1 and the unsigned values range from 0 to 2^n, so the union of the ranges is -2^(n-1) to 2^n.

I realized this code will be UB and broken when SIZE is 64 because the 1 << 64 is nonsense, so I will have to fix and test that before I land this.

aheejin added inline comments.Jul 18 2023, 1:33 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
53

Then are immediates in SIMD signed or unsigned?

Peter added a subscriber: Peter.Sep 22 2023, 1:14 PM

Hi all, thanks for the patch. Are there any updates on this patch? Do we plan to merge it to upstream?

Herald added a subscriber: sunshaoce. · View Herald TranscriptSep 22 2023, 1:14 PM
Peter added inline comments.Sep 22 2023, 1:19 PM
llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td
53

I think you can use less equal 2^n -1 to avoid UB. The expression is complicated but const expr can be folded during compilation I think.

Imm <= ((1 << ("#SIZE#" - 1)) - 1) * 2 + 1

Revision Contents

PathSize
llvm/
lib/
Target/
WebAssembly/
5 lines
test/
CodeGen/
WebAssembly/
15 lines

Diff 540754

llvm/lib/Target/WebAssembly/WebAssemblyInstrSIMD.td

Show All 40 Lines
defm "" : ARGUMENT<V128, v16i8>; defm "" : ARGUMENT<V128, v16i8>;
defm "" : ARGUMENT<V128, v8i16>; defm "" : ARGUMENT<V128, v8i16>;
defm "" : ARGUMENT<V128, v4i32>; defm "" : ARGUMENT<V128, v4i32>;
defm "" : ARGUMENT<V128, v2i64>; defm "" : ARGUMENT<V128, v2i64>;
defm "" : ARGUMENT<V128, v4f32>; defm "" : ARGUMENT<V128, v4f32>;
defm "" : ARGUMENT<V128, v2f64>; defm "" : ARGUMENT<V128, v2f64>;
// Constrained immediate argument types // Constrained immediate argument types. Allow any value from the minimum signed
// value to the maximum unsigned value for the lane size.
foreach SIZE = [8, 16] in foreach SIZE = [8, 16] in
def ImmI#SIZE : ImmLeaf<i32, def ImmI#SIZE : ImmLeaf<i32,
"return -(1 << ("#SIZE#" - 1)) <= Imm && Imm < (1 << ("#SIZE#" - 1));" "return -(1 << ("#SIZE#" - 1)) <= Imm && Imm < (1 << "#SIZE#");"
aheejinUnsubmitted
Not Done
ReplyInline Actions

Why are the negative range and the positive range different?

aheejin: Why are the negative range and the positive range different?
tlivelyAuthorUnsubmitted
Done
ReplyInline Actions

The signed values range from -2^(n-1) to 2^(n-1)-1 and the unsigned values range from 0 to 2^n, so the union of the ranges is -2^(n-1) to 2^n.

I realized this code will be UB and broken when SIZE is 64 because the 1 << 64 is nonsense, so I will have to fix and test that before I land this.

tlively: The signed values range from -2^(n-1) to 2^(n-1)-1 and the unsigned values range from 0 to 2^n…
aheejinUnsubmitted
Not Done
ReplyInline Actions

Then are immediates in SIMD signed or unsigned?

aheejin: Then are immediates in SIMD signed or unsigned?
PeterUnsubmitted
Not Done
ReplyInline Actions

I think you can use less equal 2^n -1 to avoid UB. The expression is complicated but const expr can be folded during compilation I think.

Imm <= ((1 << ("#SIZE#" - 1)) - 1) * 2 + 1

Peter: I think you can use less equal 2^n -1 to avoid UB. The expression is complicated but const expr…
>; >;
foreach SIZE = [2, 4, 8, 16, 32] in foreach SIZE = [2, 4, 8, 16, 32] in
def LaneIdx#SIZE : ImmLeaf<i32, "return 0 <= Imm && Imm < "#SIZE#";">; def LaneIdx#SIZE : ImmLeaf<i32, "return 0 <= Imm && Imm < "#SIZE#";">;
class Vec { class Vec {
ValueType vt; ValueType vt;
ValueType int_vt; ValueType int_vt;
ValueType lane_vt; ValueType lane_vt;
▲ Show 20 Lines • Show All 1,445 Lines • Show Last 20 Lines

llvm/test/CodeGen/WebAssembly/pr63817.ll

  • This file was added.
; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
; RUN: llc < %s -mtriple=wasm32 -mattr=+simd128 | FileCheck %s
;; Regression test for a bug in which BUILD_VECTOR nodes with large unsigned
;; lane constants were not properly selected.
define <4 x i8> @test(<4 x i8> %0) {
; CHECK-LABEL: test:
; CHECK: .functype test (v128) -> (v128)
; CHECK-NEXT: # %bb.0:
; CHECK-NEXT: v128.const 255, 17, 255, 255, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0
; CHECK-NEXT: # fallthrough-return
%V1 = or <4 x i8> <i8 255, i8 255, i8 255, i8 255>, %0
%V2 = insertelement <4 x i8> %V1, i8 17, i32 1
ret <4 x i8> %V2
}