Skip to content

Conversation

@troughton
Copy link
Contributor

On Windows 64-bit, the dividingFullWidth method on UInt64 and Int64 causes LLVM to emit references to udivti3, umodti3, divti3, and modti3, which are undefined on Windows. Provide an implementation here based on Compiler-RT.

This is an alternative fix to #13234.

cc @gparker42

and use unsigned int directly instead of Compiler-RT's equivalent su_int.
#define SWIFT_MODE_TI __attribute__((__mode__(TI)))
#else
#define SWIFT_MODE_DI
#define SWIFT_MODE_TI
Copy link
Member

Choose a reason for hiding this comment

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

Please make this a hard error. Without this, the types are completely the wrong size and you do not get any type of warning/error that things went wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem with a hard error is that I moved this out of the #if blocks, since the definitions are used by multiple disjoint if blocks. If we hard error, then that applies to every platform, even ones that will never need to use the types.

The alternatives would be to either warn or to duplicate these definitions and move them back into each #if block. The problem that then arises is that the definitions become conditional on what #if blocks we'd already encountered, unless we #defined and #undefd the types within each block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this by adding a 3-line check for __mode__ support within each #if block.


#if defined(_WIN64)

#define CHAR_BIT 8 // number of bits in a char
Copy link
Member

Choose a reason for hiding this comment

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

Can we not use the standard headers for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, sorry. I was blindly following Compiler-RT here, but it's already defined from <limits.h>.

du_int low;
du_int high;
}s;
} utwords;
Copy link
Member

Choose a reason for hiding this comment

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

This technically is a GNU extension (union fields may only be access through initialized fields) and breaks aliasing. At that point, you should assume that the __mode__ extension is available

@troughton
Copy link
Contributor Author

@gparker42 When you get a chance, could you take a look at this in comparison to #13234 and see which approach you prefer?

@xwu
Copy link
Collaborator

xwu commented Jan 20, 2018

Currently, dividingFullWidth uses DoubleWidth to perform the operation for 64-bit values on all platforms, so I think the issue with Win64 no longer applies. I'm also planning to revamp division in DoubleWidth shortly (in #13784) to be more efficient.

@troughton
Copy link
Contributor Author

Okay, great. I’ll close these two PRs.

@troughton troughton closed this Jan 20, 2018
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.

3 participants