New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cannot compile to wasm with multivalue support #55136
Comments
Files from |
@llvm/issue-subscribers-backend-webassembly |
I reduced the reproducer to this IR: target triple = "wasm32-unknown-emscripten"
%i64_struct = type { i64 }
%pair = type { i32, %i64_struct }
declare %pair @return_struct() #0
define void @test() #1 {
%p = tail call %pair @return_struct()
%first = extractvalue %pair %p, 0
%new_p = insertvalue %pair undef, i32 %first, 0
%second = extractvalue %pair %p, 1
%new_p2 = insertvalue %pair %new_p, %i64_struct %second, 1
%new_first = extractvalue %pair %new_p2, 0
%new_second = extractvalue %pair %new_p2, 1
%inner = extractvalue %i64_struct %new_second, 0
store i32 %new_first, i32* undef, align 8
store i64 %inner, i64* null, align 8
unreachable
}
attributes #0 = { readnone }
attributes #1 = { "target-features"="+multivalue" } with this stack trace:
|
Hi, I started looking into this, but don't have any experience with LLVM development so my methodology might be all over the place :) The reproducer can be further reduced to this:target triple = "wasm32-unknown-unknown"
%pair = type { i32, i64 }
declare %pair @return_struct() #0 readnone
define void @test() #0 {
%p = call %pair @return_struct()
%first = extractvalue %pair %p, 0
%second = extractvalue %pair %p, 1
store i32 %first, i32* undef, align 8
store i64 %second, i64* null, align 8
unreachable
}
attributes #0 = { "target-features"="+multivalue" } To further minimize, I simplified the
|
// If any subsequent def is used prior to the current value by the same | |
// instruction in which the current value is used, we cannot | |
// stackify. Stackifying in this case would require that def moving below the | |
// current def in the stack, which cannot be achieved, even with locals. | |
for (const auto &SubsequentDef : drop_begin(DefI->defs())) { | |
for (const auto &PriorUse : UseI->uses()) { | |
if (&PriorUse == Use) | |
break; | |
if (PriorUse.isReg() && SubsequentDef.getReg() == PriorUse.getReg()) | |
return false; | |
} | |
} |
In this example, the def and use registers don't match, and the prior use is ignored. I'm not sure why the code tests for equal registers (how can any use be valid if the def is moved below?), but that's about as far as I got.
I applied this patch, and the testsuite seems fine, though there are unrelated failing tests that might be caused by -DLLVM_ENABLE_EXPENSIVE_CHECKS=True
(?):
diff --git a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
index d3ad47147ac8..e016bacbd80f 100644
--- a/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
+++ b/llvm/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp
@@ -337,11 +337,18 @@ static bool isSafeToMove(const MachineOperand *Def, const MachineOperand *Use,
// instruction in which the current value is used, we cannot
// stackify. Stackifying in this case would require that def moving below the
// current def in the stack, which cannot be achieved, even with locals.
for (const auto &SubsequentDef : drop_begin(DefI->defs())) {
for (const auto &PriorUse : UseI->uses()) {
if (&PriorUse == Use)
break;
- if (PriorUse.isReg() && SubsequentDef.getReg() == PriorUse.getReg())
+ if (PriorUse.isReg())
return false;
}
}
Is this the correct way to run all relevant tests?
⋊> ~/_/_/llvm-project on main ⨯ ./build/bin/llvm-lit llvm/test/CodeGen/WebAssembly/ -q
********************
Failed Tests (2):
LLVM :: CodeGen/WebAssembly/dbgvalue.ll
LLVM :: CodeGen/WebAssembly/stackified-debug.ll
Failed: 2
Nice work! Here's the command I use to run all the Wasm tests from the entire project:
But I would expect that your simpler test command would be sufficient for this particular issue. The My guess is that the tests are failing because the order of emitted instructions has changed with your patch. It looks like the intention of IIUC, that block of code you identified is checking for situations like this:
Where the uses are all in the same instruction. We may need entirely new logic to fix the case in this bug where the uses are in two different instructions. This move might be allowable, but only if we can then move the store at |
I have encountered the same issue. It is triggered in TinyGo when I tried to enable multivalue support. Here is a Compiler Explorer reproducer: https://llvm.godbolt.org/z/W8KxTPz6n Reproducer in IR: source_filename = "main"
target datalayout = "e-m:e-p:32:32-p10:8:8-p20:8:8-i64:64-n32:64-S128-ni:1:10:20"
target triple = "wasm32-unknown-wasi"
declare { i32, i32 } @foo() #0
define i32 @crashing() #0 {
entry:
%foo1 = call { i32, i32 } @foo()
%foo2 = call { i32, i32 } @foo()
%foo1_0 = extractvalue { i32, i32 } %foo2, 0
%foo1_1 = extractvalue { i32, i32 } %foo2, 1
%foo2_0 = extractvalue { i32, i32 } %foo1, 0
%a = add i32 %foo2_0, %foo1_1
%b = add i32 %a, %foo1_0
ret i32 %b
}
attributes #0 = { "target-features"="+multivalue" } |
Fixed in commit b303c0027ff7f3a9912d7690886b7b7b33ddb05f |
Trying to use
mpack.c
with multivalue support. I have some code that uses mpack, but the error comes directly from there and doesn't seem to depend on what I do in my code:The text was updated successfully, but these errors were encountered: