Skip to content
This repository has been archived by the owner on Oct 12, 2022. It is now read-only.

Fix Issue 19204 - hashOf doesn't accept SIMD vectors #2289

Merged
merged 1 commit into from Oct 24, 2018

Conversation

n8sh
Copy link
Member

@n8sh n8sh commented Aug 29, 2018

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @n8sh!

Bugzilla references

Auto-close Bugzilla Severity Description
19204 normal hashOf doesn't accept SIMD vectors

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + druntime#2289"

@dlang-bot dlang-bot added the Bug Fix Include reference to corresponding bugzilla issue label Aug 29, 2018
@n8sh n8sh force-pushed the core-hash-vector branch 2 times, most recently from 0a676a1 to 0d43981 Compare August 29, 2018 11:24
src/core/internal/convert.d Outdated Show resolved Hide resolved
src/core/internal/hash.d Outdated Show resolved Hide resolved
@n8sh n8sh force-pushed the core-hash-vector branch 8 times, most recently from bc15a0a to 82e0c70 Compare September 5, 2018 08:22
@n8sh
Copy link
Member Author

n8sh commented Sep 5, 2018

In trying to get CTFE to work ran into https://issues.dlang.org/show_bug.cgi?id=19223.

EDIT: Made a separate bug report https://issues.dlang.org/show_bug.cgi?id=19224 solely for the CTFE failure since it might not have the same cause.

@n8sh n8sh force-pushed the core-hash-vector branch 6 times, most recently from d87df32 to cc423f8 Compare September 5, 2018 16:55
@n8sh
Copy link
Member Author

n8sh commented Oct 6, 2018

This needs to be merged before dlang/dmd#8676 can be merged, or else any program that has a struct that has a vector field and for any reason needs to generate __xtoHash will fail to compile.

@n8sh n8sh force-pushed the core-hash-vector branch 3 times, most recently from 8aac7c3 to 4f265e1 Compare October 6, 2018 14:41
@n8sh n8sh force-pushed the core-hash-vector branch 2 times, most recently from 76a407b to 53fdaf8 Compare October 10, 2018 23:18
@n8sh
Copy link
Member Author

n8sh commented Oct 10, 2018

CTFE now works for float vectors.

static if (is(simd.float4)) // __traits(isArithmetic) and __traits(isFloating)
{{
enum simd.float4 val = [1.1f, 2.2f, 3.3f, 4.4f];
enum ctfeHash = hashOf!(simd.float4)(val);
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need to explicitly instantiate hashOf here?

Copy link
Member Author

@n8sh n8sh Oct 24, 2018

Choose a reason for hiding this comment

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

Didn't need to.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, I did need to. It was long enough ago that I forgot. Without the explicit instantiation the type is inferred as a floating point array.

size_t result = seed;
foreach (x; val.array)
result = hashOf(x, result);
return result;
Copy link
Member

Choose a reason for hiding this comment

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

Why not keep only the CTFE version?

Copy link
Member Author

@n8sh n8sh Oct 24, 2018

Choose a reason for hiding this comment

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

It (EDIT: the non-CTFE version) seemed to produce better code with DMD. It looks like LDC2 compiles them both the same though.

@n8sh n8sh force-pushed the core-hash-vector branch 2 times, most recently from 3053fcb to af56c45 Compare October 24, 2018 08:04
@n8sh
Copy link
Member Author

n8sh commented Oct 24, 2018

The explicit instantiation in the CTFE test was needed because at compile time the type was inferred as float[4] rather than __vector(float[4]). To work around this I'm making the SIMD hash behave the same as for a static array, although for floating-point types this is currently slightly slower.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug Fix Include reference to corresponding bugzilla issue
Projects
None yet
5 participants