Skip to content

[stdlib/Sema] Minor improvements to synthesized hashValue #14386

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Feb 5, 2018
Merged

[stdlib/Sema] Minor improvements to synthesized hashValue #14386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 10 additions & 20 deletions lib/Sema/DerivedConformanceEquatableHashable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -838,7 +838,7 @@ deriveBodyHashable_enum_hashValue(AbstractFunctionDecl *hashValueDecl) {
for (auto elt : enumDecl->getAllElements()) {
// case .<elt>(let a0, let a1, ...):
SmallVector<VarDecl*, 3> payloadVars;
SmallVector<ASTNode, 3> mixExpressions;
SmallVector<ASTNode, 3> combineExprs;

auto payloadPattern = enumElementPayloadSubpattern(elt, 'a', hashValueDecl,
payloadVars);
Expand All @@ -863,7 +863,7 @@ deriveBodyHashable_enum_hashValue(AbstractFunctionDecl *hashValueDecl) {
/*implicit*/ true);
auto assignExpr = new (C) AssignExpr(resultRef, SourceLoc(),
ordinalExpr, /*implicit*/ true);
mixExpressions.emplace_back(ASTNode(assignExpr));
combineExprs.emplace_back(ASTNode(assignExpr));
}

if (!hasNoAssociatedValues) {
Expand All @@ -873,18 +873,14 @@ deriveBodyHashable_enum_hashValue(AbstractFunctionDecl *hashValueDecl) {
auto payloadVarRef = new (C) DeclRefExpr(payloadVar, DeclNameLoc(),
/*implicit*/ true);
// result = _combineHashValues(result, <payloadVar>.hashValue)
auto mixExpr = combineHashValuesAssignmentExpr(C, resultVar,
payloadVarRef);
mixExpressions.emplace_back(ASTNode(mixExpr));
auto combineExpr = combineHashValuesAssignmentExpr(C, resultVar,
payloadVarRef);
combineExprs.emplace_back(ASTNode(combineExpr));
}

// result = _mixInt(result)
auto assignExpr = mixIntAssignmentExpr(C, resultVar);
mixExpressions.emplace_back(ASTNode(assignExpr));
}

auto hasBoundDecls = !payloadVars.empty();
auto body = BraceStmt::create(C, SourceLoc(), mixExpressions, SourceLoc());
auto body = BraceStmt::create(C, SourceLoc(), combineExprs, SourceLoc());
cases.push_back(CaseStmt::create(C, SourceLoc(), labelItem, hasBoundDecls,
SourceLoc(), body));
}
Expand Down Expand Up @@ -963,16 +959,12 @@ deriveBodyHashable_struct_hashValue(AbstractFunctionDecl *hashValueDecl) {
auto selfPropertyExpr = new (C) DotSyntaxCallExpr(propertyRef, SourceLoc(),
selfRef);
// result = _combineHashValues(result, <property>.hashValue)
auto mixExpr = combineHashValuesAssignmentExpr(C, resultVar,
selfPropertyExpr);
statements.emplace_back(ASTNode(mixExpr));
auto combineExpr = combineHashValuesAssignmentExpr(C, resultVar,
selfPropertyExpr);
statements.emplace_back(ASTNode(combineExpr));
}

{
// result = _mixInt(result)
auto assignExpr = mixIntAssignmentExpr(C, resultVar);
statements.push_back(assignExpr);

// return result
auto resultRef = new (C) DeclRefExpr(resultVar, DeclNameLoc(),
/*implicit*/ true,
Expand Down Expand Up @@ -1021,7 +1013,6 @@ deriveHashable_hashValue(TypeChecker &tc, Decl *parentDecl,
// result = _combineHashValues(result, a0.hashValue)
// result = _combineHashValues(result, a1.hashValue)
// }
// result = _mixInt(result)
// return result
// }
// }
Expand All @@ -1030,10 +1021,9 @@ deriveHashable_hashValue(TypeChecker &tc, Decl *parentDecl,
// var x: Int
// var y: String
// @derived var hashValue: Int {
// var result: Int = 0
// var result = 0
// result = _combineHashValues(result, x.hashValue)
// result = _combineHashValues(result, y.hashValue)
// result = _mixInt(result)
// return result
// }
// }
Expand Down
1 change: 0 additions & 1 deletion stdlib/public/core/DoubleWidth.swift.gyb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,6 @@ extension DoubleWidth : Hashable {
var result = 0
result = _combineHashValues(result, _storage.high.hashValue)
result = _combineHashValues(result, _storage.low.hashValue)
result = _mixInt(result)
return result
}
}
Expand Down
8 changes: 7 additions & 1 deletion stdlib/public/core/Hashing.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,13 @@ func _squeezeHashValue(_ hashValue: Int, _ upperBound: Int) -> Int {
@_transparent
public // @testable
func _combineHashValues(_ firstValue: Int, _ secondValue: Int) -> Int {
let magic = 0x9e3779b9 as UInt // Based on the golden ratio.
// Use a magic number based on the golden ratio
// (0x1.9e3779b97f4a7c15f39cc0605cedc8341082276bf3a27251f86c6a11d0c18e95p0).
#if arch(i386) || arch(arm)
let magic = 0x9e3779b9 as UInt
#elseif arch(x86_64) || arch(arm64) || arch(powerpc64) || arch(powerpc64le) || arch(s390x)
let magic = 0x9e3779b97f4a7c15 as UInt
#endif
Copy link
Member

@natecook1000 natecook1000 Feb 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make sense to have compiler directives like #if archbits(32) ... #elseif archbits(64) ... #endif? Seems problematic/error prone to list individual architectures.

Copy link
Collaborator Author

@xwu xwu Feb 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We can already write this as if MemoryLayout<Int>.size == 4, etc., and the compiler takes care of optimizing it. But here I'm following the prevailing style in this particular file.

Edit: Alternatively, a simplification which could be done down the road is to align ourselves with the assumption (already made elsewhere in the project) that only i386 and arm are 32-bit.

Copy link
Member

Choose a reason for hiding this comment

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

Is it smart enough not to complain that the 64-bit value is out of bounds on 32-bit platforms if you write that?

I'm not critiquing what you did here — more just wondering how this could be more clear. Totally possible that this is basically only an issue in places like this in the standard library.

Copy link
Collaborator Author

@xwu xwu Feb 3, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

That’s a good point; the code may not actually compile on a 32-bit system depending on compiler smarts. I do know it’s optimized down to a constant on my 64-bit system.

I think, ultimately, the utility of another compilation conditional will come down to whether or not we realistically expect Swift to support 32–bit architectures other than i386 and arm. It’s not entirely out of the question but does seem pretty unlikely in the near term.

Copy link
Member

@lorentey lorentey Feb 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

A dedicated directive would certainly express the underlying intention better, even if we never add support for additional architectures. We already have _endian(little)/_endian(big) for endianness.

In any case, for now, we could replace the #elseif with #else here; the compiler will complain if the constant doesn't fit in an UInt on some new 32-bit architecture. (It seems unnecessarily fragile/verbose to list all supported 64-bit architectures, although it is arguably the right choice in the rest of the file. We did go with #else in the new String implementation, although that may have just been due to laziness on my part. 🙈)

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 agree that it's a pragmatic solution for the time being. I'll create a follow-up PR with that change.

var x = UInt(bitPattern: firstValue)
x ^= UInt(bitPattern: secondValue) &+ magic &+ (x &<< 6) &+ (x &>> 2)
return Int(bitPattern: x)
Expand Down