Skip to content
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

Cranelift: Fuzz failure with egraphs on AArch64 #5405

Closed
afonso360 opened this issue Dec 9, 2022 · 1 comment · Fixed by #5409
Closed

Cranelift: Fuzz failure with egraphs on AArch64 #5405

afonso360 opened this issue Dec 9, 2022 · 1 comment · Fixed by #5409
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator

Comments

@afonso360
Copy link
Contributor

👋 Hey,

The fuzzer found this error overnight on AArch64. It passes on x86.

.clif Test Case

test interpret
test run
set opt_level=speed_and_size
set use_egraphs=true
target aarch64

function %a(i64) -> i8 system_v {
block0(v0: i64):
    v6 = iconst.i8 51
    v17 = imul v6, v6  ; v6 = 51, v6 = 51
    v18 = icmp eq v17, v17
    v52 = imul v18, v18
    return v52
}

; run: %a(129) == 1

Steps to Reproduce

  • clif-util ./the-above.clif

Expected Results

The test to pass

Actual Results

afonso@DESKTOP-VSTS4BC:~/git/wasmtime/cranelift$ cargo run --target aarch64-unknown-linux-gnu -- test ./lmao.clif 
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `qemu-aarch64 -L /usr/aarch64-linux-gnu -E LD_LIBRARY_PATH=/usr/aarch64-linux-gnu/lib /home/afonso/git/wasmtime/target/aarch64-unknown-linux-gnu/debug/clif-util test ./lmao.clif`
 ERROR cranelift_filetests::concurrent > FAIL: run
FAIL ./lmao.clif: run

Caused by:
    Failed test: run: %a(129) == 1, actual: 0
1 tests
Error: 1 failure

Versions and Environment

Cranelift version or commit: main
Operating system: linux
Architecture: aarch64

cc: @cfallin

@afonso360 afonso360 added bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator labels Dec 9, 2022
@cfallin
Copy link
Member

cfallin commented Dec 9, 2022

Seems this is an issue with narrow-types masking and the constant propagation rules in cprop.isle: 51*51 (which is 2601) was rewritten to iconst.i8 2601 rather than iconst.i8 41 (2601 & 0xff). On aarch64 this lowers to an immediate-value sequence that produces 41 and a compare that compares against 2601.

Separately we should extend cprop so it can recognize icmp of two equal constants, and algebraic rules so it can recognize icmp eq x, x; either one would have optimized this case away.

cfallin added a commit to cfallin/wasmtime that referenced this issue Dec 9, 2022
This fixes bytecodealliance#5405.

In the egraph mid-end's optimization rules, we were rewriting e.g. imuls
of two iconsts to an iconst of the result, but without masking off the
high bits (beyond the result type's width). This was producing iconsts
with set high bits beyond their types' width, which is not legal.

In addition, this PR adds some optimizations to the algebraic rules to
recognize e.g. `x == x` (and all other integer comparison operators) and
resolve to 1 or 0 as appropriate.
cfallin added a commit to cfallin/wasmtime that referenced this issue Dec 9, 2022
This fixes bytecodealliance#5405.

In the egraph mid-end's optimization rules, we were rewriting e.g. imuls
of two iconsts to an iconst of the result, but without masking off the
high bits (beyond the result type's width). This was producing iconsts
with set high bits beyond their types' width, which is not legal.

In addition, this PR adds some optimizations to the algebraic rules to
recognize e.g. `x == x` (and all other integer comparison operators) and
resolve to 1 or 0 as appropriate.
cfallin added a commit that referenced this issue Dec 9, 2022
…5409)

* Fix optimization rules for narrow types: wrap i8 results to 8 bits.

This fixes #5405.

In the egraph mid-end's optimization rules, we were rewriting e.g. imuls
of two iconsts to an iconst of the result, but without masking off the
high bits (beyond the result type's width). This was producing iconsts
with set high bits beyond their types' width, which is not legal.

In addition, this PR adds some optimizations to the algebraic rules to
recognize e.g. `x == x` (and all other integer comparison operators) and
resolve to 1 or 0 as appropriate.

* Review feedback.

* Review feedback, again.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants