Skip to content

Potentially wrong SIMD opcode generation in 0.17.2 #1551

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

Closed
postspectacular opened this issue Nov 20, 2020 · 5 comments · Fixed by #1552
Closed

Potentially wrong SIMD opcode generation in 0.17.2 #1551

postspectacular opened this issue Nov 20, 2020 · 5 comments · Fixed by #1552
Labels

Comments

@postspectacular
Copy link

postspectacular commented Nov 20, 2020

Hi all, I'm not sure if that's related to the recent Binaryen updates in 0.17.2, but the following code (which compiles just fine w/ all recent assemblyscript versions < 0.17.2) is now causing a validation error during compile and seemingly produces the wrong SIMD opcodes to begin with:

https://github.com/thi-ng/umbrella/blob/develop/packages/simd/assembly/abs.ts#L12

export function abs4_f32(
    out: usize,
    a: usize,
    num: usize,
    so: usize,
    sa: usize
): usize {
    so <<= 2;
    sa <<= 2;
    const res = out;
    for (; num-- > 0; ) {
        v128.store(out, f32x4.abs(v128.load(a)));
        out += so;
        a += sa;
    }
    return res;
}
[wasm-validator error in function assembly/abs/abs4_f32] i32 != v128: store value type must match, on 
[none] (v128.store
 [i32] (local.get $0)
 [i32] (i64x2.all_true
  [v128] (v128.load
   [i32] (local.get $1)
  )
 )
)
FAILURE validate error
    at Object.main (/Users/thing/umbrella/node_modules/assemblyscript/cli/asc.js:735:23)
    at /Users/thing/umbrella/node_modules/assemblyscript/bin/asc:21:47

Where is that i64x2.all_true op coming from here? Should be f32x4.abs...

@dcodeIO
Copy link
Member

dcodeIO commented Nov 20, 2020

Indeed looks like another renumbering took place there and we missed to update the constants. cc @MaxGraey

@MaxGraey
Copy link
Member

I think we should update constants every time after update binaryen. @dcodeIO could you remind me about this during review?

@dcodeIO
Copy link
Member

dcodeIO commented Nov 20, 2020

I guess we could build a test case that compares the constants somehow :)

@postspectacular
Copy link
Author

Thank you both! Is that maybe something which could be semi-automated via a little dev tool? I'm assuming those constants are retrieved from somewhere in the Binaryen codebase. Want me to look into it? If so, can you please provide some more pointers, i.e. which files & enums should be considered for syncing...

@dcodeIO
Copy link
Member

dcodeIO commented Nov 20, 2020

The script responsible for updating the constants is https://github.com/AssemblyScript/assemblyscript/blob/master/scripts/update-constants.js

Perhaps it could provide a --check option that doesn't update, but just compares, yielding an error with a non zero exit code on mismatch. Would then be easy to integrate into CI :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants