[WebAssembly] Correctly consider signext/zext arg flags at function declaration #77281

aqjune · 2024-01-08T08:06:12Z

This patch fixes WebAssembly's FastISel pass to correctly consider signext/zeroext parameter flags at function declaration.
Previously, the flags at call sites were only considered during code generation, which caused an interesting bug report #63388 .
This is problematic especially because in WebAssembly's ABI, either signext or zeroext can be tagged to a function argument, and it must be correctly reflected in the generated code. Unit test https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/WebAssembly/signext-zeroext.ll shows that i8 zeroext %t and i8 signext %t's code gen are different.

…eclaration

aqjune · 2024-01-08T08:06:31Z

llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp

@@ -839,9 +839,11 @@ bool WebAssemblyFastISel::selectCall(const Instruction *I) {

    unsigned Reg;

-    if (Attrs.hasParamAttr(I, Attribute::SExt))
+    if (Attrs.hasParamAttr(I, Attribute::SExt) ||
+        (IsDirect && Func->hasParamAttribute(I, Attribute::SExt)))


IsDirect is true if the function call isn't an indirect call.

I have been too long away from LLVM to LGTM, but I would guess that you would have a similar situation with indirect function calls as well. Perhaps you could test a function that takes a function pointer parameter with sext / zext attributes. But, again, perhaps this comment is off-base.

Pointers are (on their way to being) opaque; I forget whether function pointers ever had these parameters attached, but in any case they soon won't, so for indirect calls I think it should just be the instruction.

llvmbot · 2024-01-08T08:06:43Z

@llvm/pr-subscribers-backend-webassembly

Author: Juneyoung Lee (aqjune)

Changes

This patch fixes WebAssembly's FastISel pass to correctly consider signext/zeroext parameter flags at function declaration.
Previously, the flags at call sites were only considered during code generation, which caused an interesting bug report #63388 .
This is problematic especially because in WebAssembly's ABI, either signext or zeroext can be tagged to a function argument, and it must be correctly reflected in the generated code. Unit test https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/WebAssembly/signext-zeroext.ll shows that i8 zeroext %t and i8 signext %t's code gen are different.

Full diff: https://github.com/llvm/llvm-project/pull/77281.diff

2 Files Affected:

(modified) llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp (+4-2)
(added) llvm/test/CodeGen/WebAssembly/signext-zeroext-callsite.ll (+125)

diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
index 15dc44a0439573..80159974ecd691 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyFastISel.cpp
@@ -839,9 +839,11 @@ bool WebAssemblyFastISel::selectCall(const Instruction *I) {
 
     unsigned Reg;
 
-    if (Attrs.hasParamAttr(I, Attribute::SExt))
+    if (Attrs.hasParamAttr(I, Attribute::SExt) ||
+        (IsDirect && Func->hasParamAttribute(I, Attribute::SExt)))
       Reg = getRegForSignedValue(V);
-    else if (Attrs.hasParamAttr(I, Attribute::ZExt))
+    else if (Attrs.hasParamAttr(I, Attribute::ZExt) ||
+             (IsDirect && Func->hasParamAttribute(I, Attribute::ZExt)))
       Reg = getRegForUnsignedValue(V);
     else
       Reg = getRegForValue(V);
diff --git a/llvm/test/CodeGen/WebAssembly/signext-zeroext-callsite.ll b/llvm/test/CodeGen/WebAssembly/signext-zeroext-callsite.ll
new file mode 100644
index 00000000000000..02ca578716dc98
--- /dev/null
+++ b/llvm/test/CodeGen/WebAssembly/signext-zeroext-callsite.ll
@@ -0,0 +1,125 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -O0 | FileCheck %s
+
+target triple = "wasm32-unknown-unknown"
+
+
+declare i32 @foo(i1 signext noundef, i32 noundef)
+
+; callsite_signext and callsite_nosignext must emit equivalent codes
+
+define i32 @callsite_nosignext() {
+; CHECK-LABEL: callsite_nosignext:
+; CHECK:         .functype callsite_nosignext () -> (i32)
+; CHECK-NEXT:    .local i32, i32, i32, i32, i32, i32
+; CHECK-NEXT:  # %bb.0: # %start
+; CHECK-NEXT:    i32.const 1
+; CHECK-NEXT:    local.set 0
+; CHECK-NEXT:    i32.const 0
+; CHECK-NEXT:    local.set 1
+; CHECK-NEXT:    i32.const 31
+; CHECK-NEXT:    local.set 2
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    local.get 2
+; CHECK-NEXT:    i32.shl
+; CHECK-NEXT:    local.set 3
+; CHECK-NEXT:    local.get 3
+; CHECK-NEXT:    local.get 2
+; CHECK-NEXT:    i32.shr_s
+; CHECK-NEXT:    local.set 4
+; CHECK-NEXT:    local.get 4
+; CHECK-NEXT:    local.get 1
+; CHECK-NEXT:    call foo
+; CHECK-NEXT:    local.set 5
+; CHECK-NEXT:    local.get 5
+; CHECK-NEXT:    return
+start:
+  %0 = call i32 @foo(i1 1, i32 0)
+  ret i32 %0
+}
+
+define i32 @callsite_signext() {
+; CHECK-LABEL: callsite_signext:
+; CHECK:         .functype callsite_signext () -> (i32)
+; CHECK-NEXT:    .local i32, i32, i32, i32, i32, i32
+; CHECK-NEXT:  # %bb.0: # %start
+; CHECK-NEXT:    i32.const 1
+; CHECK-NEXT:    local.set 0
+; CHECK-NEXT:    i32.const 0
+; CHECK-NEXT:    local.set 1
+; CHECK-NEXT:    i32.const 31
+; CHECK-NEXT:    local.set 2
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    local.get 2
+; CHECK-NEXT:    i32.shl
+; CHECK-NEXT:    local.set 3
+; CHECK-NEXT:    local.get 3
+; CHECK-NEXT:    local.get 2
+; CHECK-NEXT:    i32.shr_s
+; CHECK-NEXT:    local.set 4
+; CHECK-NEXT:    local.get 4
+; CHECK-NEXT:    local.get 1
+; CHECK-NEXT:    call foo
+; CHECK-NEXT:    local.set 5
+; CHECK-NEXT:    local.get 5
+; CHECK-NEXT:    return
+start:
+  %0 = call i32 @foo(i1 signext 1, i32 0)
+  ret i32 %0
+}
+
+declare i32 @foo2(i1 zeroext noundef, i32 noundef)
+
+; callsite_zeroext and callsite_nozeroext must emit equivalent codes
+
+define i32 @callsite_nozeroext() {
+; CHECK-LABEL: callsite_nozeroext:
+; CHECK:         .functype callsite_nozeroext () -> (i32)
+; CHECK-NEXT:    .local i32, i32, i32, i32, i32
+; CHECK-NEXT:  # %bb.0: # %start
+; CHECK-NEXT:    i32.const 1
+; CHECK-NEXT:    local.set 0
+; CHECK-NEXT:    i32.const 0
+; CHECK-NEXT:    local.set 1
+; CHECK-NEXT:    i32.const 1
+; CHECK-NEXT:    local.set 2
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    local.get 2
+; CHECK-NEXT:    i32.and
+; CHECK-NEXT:    local.set 3
+; CHECK-NEXT:    local.get 3
+; CHECK-NEXT:    local.get 1
+; CHECK-NEXT:    call foo2
+; CHECK-NEXT:    local.set 4
+; CHECK-NEXT:    local.get 4
+; CHECK-NEXT:    return
+start:
+  %0 = call i32 @foo2(i1 1, i32 0)
+  ret i32 %0
+}
+
+define i32 @callsite_zeroext() {
+; CHECK-LABEL: callsite_zeroext:
+; CHECK:         .functype callsite_zeroext () -> (i32)
+; CHECK-NEXT:    .local i32, i32, i32, i32, i32
+; CHECK-NEXT:  # %bb.0: # %start
+; CHECK-NEXT:    i32.const 1
+; CHECK-NEXT:    local.set 0
+; CHECK-NEXT:    i32.const 0
+; CHECK-NEXT:    local.set 1
+; CHECK-NEXT:    i32.const 1
+; CHECK-NEXT:    local.set 2
+; CHECK-NEXT:    local.get 0
+; CHECK-NEXT:    local.get 2
+; CHECK-NEXT:    i32.and
+; CHECK-NEXT:    local.set 3
+; CHECK-NEXT:    local.get 3
+; CHECK-NEXT:    local.get 1
+; CHECK-NEXT:    call foo2
+; CHECK-NEXT:    local.set 4
+; CHECK-NEXT:    local.get 4
+; CHECK-NEXT:    return
+start:
+  %0 = call i32 @foo2(i1 zeroext 1, i32 0)
+  ret i32 %0
+}

dschuff · 2024-01-08T22:46:32Z

llvm/test/CodeGen/WebAssembly/signext-zeroext-callsite.ll

@@ -0,0 +1,125 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -O0 | FileCheck %s


Is there a test that covers this behavior for DAG ISel? Maybe it would make sense to add test expectations for both here?

I added the -fast-isel=false case to check the case when the regular DAG ISel path is taken.

asb · 2024-01-09T09:59:57Z

In case anyone was wondering how this is handled in SelectionDAG, I believe it's covered by CallLoweringInfo ultimately determining if an arg is sext/zext through CallBase::paramHasAttr, which does indeed check both the callsite and the called function (if it's a direct call of course). Indeed, I think you could just switch to using paramHasAttr in this patch and get the desired behaviour (which seems to be what ARMFastISel does in its SelectCall implementation).

Although this does fix a real bug, I'd strongly advise that a bug be filed against whatever code generator produced a call lacking the matching signext/zeroext attribute on the caller. I don't believe that in general LLVM is particularly well tested or robust against such inconsistencies, so it's likely to run into bugs.

dschuff

Thanks for the fix!

aqjune · 2024-01-09T22:14:09Z

I will be back from office in a few hours and check whether I can use CallBase::paramHasAttr. Could you wait a bit until then?

dschuff · 2024-01-09T23:25:54Z

Yeah, sorry I missed Alex's suggestion there. There's no hurry.

aqjune · 2024-01-09T23:54:15Z

I updated it to use the paramHasAttr which passed my test as it supposed to do so. :) Thanks for suggestion.

aqjune · 2024-01-10T05:54:39Z

I checked that the CI has passed - thanks all!

…eclaration (llvm#77281) This patch fixes WebAssembly's FastISel pass to correctly consider signext/zeroext parameter flags at function declaration. Previously, the flags at call sites were only considered during code generation, which caused an interesting bug report llvm#63388 . This is problematic especially because in WebAssembly's ABI, either signext or zeroext can be tagged to a function argument, and it must be correctly reflected in the generated code. Unit test https://github.com/llvm/llvm-project/blob/main/llvm/test/CodeGen/WebAssembly/signext-zeroext.ll shows that `i8 zeroext %t` and `i8 signext %t`'s code gen are different.

[WebAssembly] Correctly consider signext/zext arg flags at function d…

1bbfe05

…eclaration

llvmbot added the backend:WebAssembly label Jan 8, 2024

aqjune commented Jan 8, 2024

View reviewed changes

aqjune mentioned this pull request Jan 8, 2024

[WASM] "signext" attribute leads miscompilation #63388

Closed

aqjune added 2 commits January 8, 2024 03:32

Merge branch 'main' into issue_63388

b6996f3

Merge branch 'main' into issue_63388

55e7da0

dschuff reviewed Jan 8, 2024

View reviewed changes

add fast-isel=false case

3d2eb31

aqjune requested review from pmatos and wingo January 9, 2024 04:40

dschuff approved these changes Jan 9, 2024

View reviewed changes

aqjune added 2 commits January 9, 2024 17:52

Use CallBase::paramHasAttr

b68bf11

Merge branch 'main' into issue_63388

499be69

Merge branch 'main' into issue_63388

212f991

aqjune merged commit 7388b74 into llvm:main Jan 10, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WebAssembly] Correctly consider signext/zext arg flags at function declaration #77281

[WebAssembly] Correctly consider signext/zext arg flags at function declaration #77281

aqjune commented Jan 8, 2024

aqjune Jan 8, 2024

wingo Jan 9, 2024

dschuff Jan 9, 2024

llvmbot commented Jan 8, 2024

dschuff Jan 8, 2024

aqjune Jan 8, 2024

asb commented Jan 9, 2024 •

edited

Loading

dschuff left a comment

aqjune commented Jan 9, 2024

dschuff commented Jan 9, 2024

aqjune commented Jan 9, 2024

aqjune commented Jan 10, 2024

		@@ -0,0 +1,125 @@
		; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
		; RUN: llc < %s -O0 \| FileCheck %s

[WebAssembly] Correctly consider signext/zext arg flags at function declaration #77281

[WebAssembly] Correctly consider signext/zext arg flags at function declaration #77281

Conversation

aqjune commented Jan 8, 2024

aqjune Jan 8, 2024

Choose a reason for hiding this comment

wingo Jan 9, 2024

Choose a reason for hiding this comment

dschuff Jan 9, 2024

Choose a reason for hiding this comment

llvmbot commented Jan 8, 2024

dschuff Jan 8, 2024

Choose a reason for hiding this comment

aqjune Jan 8, 2024

Choose a reason for hiding this comment

asb commented Jan 9, 2024 • edited Loading

dschuff left a comment

Choose a reason for hiding this comment

aqjune commented Jan 9, 2024

dschuff commented Jan 9, 2024

aqjune commented Jan 9, 2024

aqjune commented Jan 10, 2024

asb commented Jan 9, 2024 •

edited

Loading