Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

[Arm64] Add Simd HWIntrinsic test #16008

Merged
merged 3 commits into from
Feb 12, 2018

Conversation

sdmaclea
Copy link

No description provided.

@sdmaclea
Copy link
Author

@tannergooding I was wondering about you opinion about this versus #15771. In some ways it is similar, but perhaps it is a slightly different problem.

I am not sure what advantage a script might offer or whether a common script could be used to generate tests for all platforms.

{
if (!e.Message.Equals("Type is not supported."))
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't take localization into account, not sure if that's an issue.

Copy link
Author

Choose a reason for hiding this comment

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

I was aware of that. For initial testing it was sufficient to prove the message was set correctly.

I'll drop it after #16102 merges.

@tannergooding
Copy link
Member

I am not sure what advantage a script might offer or whether a common script could be used to generate tests for all platforms.

The benefit of using a script/templates for generation is that it lets us easily add new scenarios "en masse".

That is, many of the intrinsics take the same number of inputs and the only thing that differs is:

  • The method you call
  • The operation you do when validating the result

For example, Sse.Add, Sse.Multiply, Sse.Subtract, and Sse.Divide only differ in the validation by +, *, -, or /. Otherwise, they undergo all the same validation scenarios, etc.

The script and template means that I write the code once, run the script, and get accurate tests for all 4. I can then add any new scenarios to the template, run the script again, and have all 4 tests updated (and this actually scales much farther than just 4).

@tannergooding
Copy link
Member

It should definitely be possible to share the generation scripts/templates between the architectures.

@sdmaclea
Copy link
Author

sdmaclea commented Feb 1, 2018

@tannergooding Would it be useful to Merge these tests?

I could move the test body inside a

#if ARM64_SIMD_API_PENDING_APPROVAL_AND_COREFX_MERGE
#endif

@sdmaclea sdmaclea force-pushed the PR-ARM64-TEST-SIMD-HWINTRINSIC branch from 3aafeab to b31ef64 Compare February 1, 2018 17:47
@sdmaclea sdmaclea changed the title WIP No Merge [Arm64] Add HWIntrinsic test [Arm64] Add Simd HWIntrinsic test Feb 1, 2018
@sdmaclea
Copy link
Author

sdmaclea commented Feb 1, 2018

I marked the individual tests with
ARM64_SIMD_API_PENDING_APPROVAL_AND_OR_COREFX_MERGE

@CarolEidt @tannergooding
I think we should review and merge this.

  • It should pass on tip (since it doesn't do anything).
  • It passes on Arm64/Linux with modified CoreFX & ARM64_SIMD_API_PENDING_APPROVAL_AND_OR_COREFX_MERGE defined
  • It will allow the tests to be generated by scripts like Update x86 HWIntrinsic Tests #15771.
  • It will allow future tests to be reviewed as intrinsics are implemented in CoreCLR.

@tannergooding
Copy link
Member

@sdmaclea, I'll give a review pass sometime later today when I get some time.

I will defer to @CarolEidt on whether we should merge these before or after API review/approval

@sdmaclea sdmaclea force-pushed the PR-ARM64-TEST-SIMD-HWINTRINSIC branch from f05d9a4 to 45b59e4 Compare February 9, 2018 21:50
@sdmaclea
Copy link
Author

sdmaclea commented Feb 9, 2018

@CarolEidt Can you comment on the strategy of reviewing and merging these tests while they are disabled. It would allow the tests to be in the master branch to enable some collaboration.

@CarolEidt
Copy link

Can you comment on the strategy of reviewing and merging these tests while they are disabled. It would allow the tests to be in the master branch to enable some collaboration.

I think it's a reasonable thing to do.

@CarolEidt
Copy link

I've taken a look at this, and it seems fine to me. I didn't scrutinize it deeply, as I hope that we will replace this with a shared template between architectures.

@sdmaclea sdmaclea force-pushed the PR-ARM64-TEST-SIMD-HWINTRINSIC branch from 32f65a0 to 128687b Compare February 12, 2018 17:52
@sdmaclea
Copy link
Author

@CarolEidt lets merge this now that it is passing.

@CarolEidt CarolEidt merged commit ac7264c into dotnet:master Feb 12, 2018
@sdmaclea sdmaclea deleted the PR-ARM64-TEST-SIMD-HWINTRINSIC branch May 24, 2018 19:21
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.

4 participants