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

simd: apply some fixes to test generation #686

Merged
merged 7 commits into from
Dec 11, 2024
Merged

Conversation

evacchi
Copy link
Collaborator

@evacchi evacchi commented Nov 25, 2024

This fixes some aspects of the SIMD test codegen, introducing some (debatable) wider changes, so I'm going to push this as a draft, to let you judge :) some of these changes could be also applied to a utility class, but this should be the gist anyway

  • I am now passing args through an MStack so that we can push V128s onto it: the reason is that codegen might become complicated otherwise: each v128 is a long[] so, you would have to "unpack" each long to several pushes. This way you can just push it all at once.
  • I have added an Mstack.pushAll(long[]) method, so that we can easily push vectors all at once.
  • I have added an MStack.slice(start, len) method returning a copy of the array (thus, only useful for tests), so that
    a. all places expecting a given arg size (e.g. I have seen this behavior in the new tail call feature) do not get confused by a larger input array
    b. we can pass in a number of values we want to pull; this is important when there are v128 values, since they are size=2
  • I have added a WasmValueType.size() that is 2 for v128 and 1 otherwise

I have also added a few more missing (?) lanes (i64x2, F64x2) and the reverse of vecTo32(), etc...

I will add some other notes inline.

@mfirry this should unblock you!

folks, feel free to push directly to this branch to apply your fixes on top

Comment on lines 193 to 194
sb.append("Integer.parseUnsignedInt(\"" + v + "\")");
break;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to parse these into Float bits in this case, the raw integer will do


var invocationMethod = ".apply(" + args + ")";
var invocationMethod = ".apply(" + String.join(",", args) + ")";
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notice, this is not correct, but works for now. It should be adapted to do same thing as the lines below, but it required a little more refactoring, and I wanted to push this out quickly to unblock @mfirry

Copy link

cloudflare-workers-and-pages bot commented Nov 25, 2024

Deploying chicory with  Cloudflare Pages  Cloudflare Pages

Latest commit: 55c7a9f
Status: ✅  Deploy successful!
Preview URL: https://6f99da75.chicory.pages.dev
Branch Preview URL: https://simd-testgen-fixes.chicory.pages.dev

View logs

@evacchi
Copy link
Collaborator Author

evacchi commented Nov 26, 2024

here's an example of a test (simd_bitwise) after the change:

    @Test()
    @Order(9)
    @DisplayName("simd_bitwise.wast:31")
    public void test9() {
        ExportFunction varAnd = testModule0Instance.export("and");
        var args = new MStack();
        args.pushAll(i32ToVec( new long[] { Long.parseUnsignedLong("0"), Long.parseUnsignedLong("0"), Long.parseUnsignedLong("4294967295"), Long.parseUnsignedLong("4294967295") }));
        args.pushAll(i32ToVec( new long[] { Long.parseUnsignedLong("0"), Long.parseUnsignedLong("4294967295"), Long.parseUnsignedLong("0"), Long.parseUnsignedLong("4294967295") }));
        var results = varAnd.apply(args.slice(0, 4));
        var expected = new long[] {Long.parseUnsignedLong("0"), Long.parseUnsignedLong("0"), Long.parseUnsignedLong("0"), Long.parseUnsignedLong("4294967295") };
        assertArrayEquals(expected, vecTo32(results));
    }

vs before:

    @Test()
    @Order(9)
    @DisplayName("simd_bitwise.wast:31")
    public void test9() {
        ExportFunction varAnd = testModule0Instance.export("and");
        var results = varAnd.apply(new long[] { 0, 0, 4294967295, 4294967295 }, new long[] { 0, 4294967295, 0, 4294967295 });
        var expected = new long[] {Long.parseUnsignedLong("0"), Long.parseUnsignedLong("0"), Long.parseUnsignedLong("0"), Long.parseUnsignedLong("4294967295") };
        assertArrayEquals(expected, vecTo32(results));
    }

notice:

  • varAnd.apply() took 2 long arrays (incorrect)
  • we wrap the two arg vectors intoi32ToVec(), because even though it's 4 longs each, they should be treated as i32x4 consts, i.e. 4 integers for a v128 value
  • we avoid having to concat the 2 resulting long[2] arrs by pushing to a stack instead
  • 4294967295 is an integer literal so it errors out because it won't fit (you can add L, but it's more correct to treat it as unsigned)

notice that it's not strictly necessary to modify MStack, we can also implement and ad-hoc stack or utility class (e.g. "TestArgs") only for tests

@evacchi
Copy link
Collaborator Author

evacchi commented Nov 26, 2024

FYI, I'll be AFK for the rest of the week, so feel free to rebase/change/rewrite this PR 👋

@andreaTP
Copy link
Collaborator

Thanks @evacchi for the effort!
For anyone landing here, I like where this is going, and I think is close enough.
Two missing steps:

  • do not use MStack directly in the tests, if there are no cases where the types of the arguments are diverging I'd like to see something like:
var results = varAnd.apply(i32ToVec(new long[] { 0, 0, 4294967295, 4294967295 }, new long[] { 0, 4294967295, 0, 4294967295 }));
  • add some generated tests to guarantee the fix is working

if anyone wants to step up and bring this to conclusion, just drop a message! 🙏

@andreaTP
Copy link
Collaborator

andreaTP commented Nov 28, 2024

Updated to use an ArgsAdapter instead of MStack, we need to progress in arguments and local size tracking, I started here: fa6bea1

for anyone that want to take over the work.

evacchi and others added 5 commits December 6, 2024 18:06
Signed-off-by: Edoardo Vacchi <[email protected]>
# Conflicts:
#	test-gen-plugin/src/main/java/com/dylibso/chicory/maven/JavaTestGen.java
Copy link
Collaborator

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of nitpicks but this is ready to go, thanks 👍

Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi evacchi force-pushed the simd-testgen-fixes branch from 4f26a10 to 6921be7 Compare December 6, 2024 17:16
Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi evacchi marked this pull request as ready for review December 7, 2024 10:07
@@ -179,9 +179,9 @@ public void execute() throws MojoExecutionException {
includedExcludedWasts.add(includedWast);
}
}
// TODO: this mechanism fails when there are no excluded wast in one profile only
Copy link
Collaborator

@andreaTP andreaTP Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing critical, but FYI @electrum , personally I think it's fine to relax those checks as most of the spec have already been included and we will still review the incoming PRs.

Copy link
Collaborator

@andreaTP andreaTP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this is ready to go, thanks @evacchi !

@andreaTP andreaTP merged commit adee41c into main Dec 11, 2024
25 checks passed
@andreaTP andreaTP deleted the simd-testgen-fixes branch December 11, 2024 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants