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

Don't create a constant node for Vector64 #105538

Merged
merged 2 commits into from
Jul 28, 2024

Conversation

tannergooding
Copy link
Member

This fixes #105537

@tannergooding
Copy link
Member Author

tannergooding commented Jul 26, 2024

@matouskozak, is there a way to kick of the job that failed for #105537?

edit: Found the command, I had missed that this was just extra-platforms, I had misclicked and couldn't find the job name initially

@tannergooding
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matouskozak
Copy link
Member

@matouskozak, is there a way to kick of the job that failed for #105537?

edit: Found the command, I had missed that this was just extra-platforms, I had misclicked and couldn't find the job name initially

Yes, extra-platforms is fine. You can also use just runtime-ioslike which will trigger the tvos-arm64 job that is failing.

Copy link
Contributor

Tagging subscribers to this area: @fanyang-mono, @steveisok
See info in area-owners.md if you want to be subscribed.

@tannergooding
Copy link
Member Author

Looks like there's a non-trivial number of failures in the regular runs that are independent of the break found in #105537

Going to try and see if I can discern what's existing and what's new. Will end up putting a PR up to revert the change if this isn't trivial to resolve

@matouskozak
Copy link
Member

Looks like there's a non-trivial number of failures in the regular runs that are independent of the break found in #105537

Going to try and see if I can discern what's existing and what's new. Will end up putting a PR up to revert the change if this isn't trivial to resolve

I'll help with that after the run finishes. The mobile-related failures are tracked at #103472 which should help with discerning what's related.

@matouskozak
Copy link
Member

Looks like there's a non-trivial number of failures in the regular runs that are independent of the break found in #105537
Going to try and see if I can discern what's existing and what's new. Will end up putting a PR up to revert the change if this isn't trivial to resolve

I'll help with that after the run finishes. The mobile-related failures are tracked at #103472 which should help with discerning what's related.

I took a brief look and the System.Runtime.Intrinsics.Tests are no longer failing on tvos-arm64 job. However, the Microsoft.Bcl.Memory.Tests still do, crash dump. I think this failure could still be related, right?

@tannergooding
Copy link
Member Author

However, the Microsoft.Bcl.Memory.Tests still do, crash dump. I think this failure could still be related, right?

Could be as there weren't any changes to Base64UrlDecoder in the past few days, but Base64Decoder isn't actually using Vector128.Shuffle anywhere, notably. It's also a bit odd that only these tests are failing and only on this one platform.

The log doesn't appear to have any meaningful stack trace, all just indicating something fails in reflection as part of InterpretedInvoke_Method. Base64Decoder then uses a SimdShuffle helper that defers down to Vector128.ShuffleUnsafe which itself defers down to AdvSimd.Arm64.VectorTableLookup, so it "shouldn't" be hitting this code path anywhere.

Is there an easy way to get additional information/context for the failure here?

@steveisok
Copy link
Member

Is there an easy way to get additional information/context for the failure here?

Is there an actual crash in Microsoft.Bcl.Memory.Tests suite? I don't see it from the logs.

Given there are individual test failures, I would recommend running it local on an ios device and see if you can and debug. If you don't have a device, I'm sure @matouskozak could help. I don't think the failure is exclusive to tvOS (we only run a subset on ios).

@tannergooding
Copy link
Member Author

Is there an actual crash in Microsoft.Bcl.Memory.Tests suite? I don't see it from the logs.

I don't either, just the 8 test failures failing from an xunit assert.

If you don't have a device, I'm sure @matouskozak could help.

I don't have such a device, unfortunately. So any help getting a better log, disassembly, LLVM IR, etc would be appreciated

Comment on lines -1355 to -1356
all_const = false;
break;
Copy link
Member Author

Choose a reason for hiding this comment

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

This was the remaining issue.

Mono only supports 128-bit xconst today, but we're still trying to handle the case where only some inputs are constant; such as Vector128.Create(1, 2, x, 4). This was early exiting in the case that not all inputs were constant so in the main loop below we ended up getting Vector128.Create(1, 2, x, 0) instead.

Fixed it to not early exit and only skip parameters if some were constant; which lets us get the correct value instead.

@tannergooding
Copy link
Member Author

/azp run runtime-extra-platforms

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tannergooding tannergooding merged commit 0912e94 into dotnet:main Jul 28, 2024
110 of 118 checks passed
@tannergooding tannergooding deleted the mono-shuffle branch July 28, 2024 16:58
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[mono] Vector shuffle failures in Mono LLVM scenario
4 participants