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

[stdlib] Eliminate intermediate rounding error in floating-point strides (and related gardening) #13007

Merged
merged 5 commits into from Nov 29, 2017

Conversation

Projects
None yet
4 participants
@xwu
Collaborator

xwu commented Nov 18, 2017

Eliminating intermediate rounding error

This PR uses the fused multiply-add operation, where supported, to eliminate intermediate rounding error in floating-point strides. This produces more intuitive results in the case of stride(from: -0.2, to: 1, by: 0.2).

Related gardening

Max's work-in-progress to conditionally conform strides to RandomAccessCollection is (perhaps temporarily) backed out. Additional GYB acrobatics would be needed for floating-point strides so that subscript access returns the same value as on iteration. Moreover, the current implementation is also buggy with respect to count--not merely because of intermediate rounding error, but also because a sufficiently large count would not be exactly representable as a floating-point value prior to conversion to Int. Since these *Collection types are internal and of recent vintage, better to back out now pending a correct implementation (if possible).

Instead, true conditional conformances are teed up for Stride{To|Through}<T> where T : BinaryInteger. Certain implementations are altered, sometimes for correctness (most notably, StrideThrough.startIndex, which was incorrect for empty StrideThrough instances). These conditional conformances cannot be compiled on master due to SR-6474 and are disabled for now; tests as in #13022 are more appropriate once the code itself can actually be compiled.

Other changes include:

  • Simplifying the implementation of Strideable._step, which may also trivially improve performance.
  • Removing _DisabledRangeIndex_, an uninhabitable type that apparently pre-dates Never.

Resolves SR-6377.

@xwu

This comment has been minimized.

Collaborator

xwu commented Nov 18, 2017

@moiseev Mind kicking off a smoke test, maybe?

@natecook1000

This comment has been minimized.

Member

natecook1000 commented Nov 18, 2017

@swift-ci Please smoke test

@xwu xwu changed the title from [stdlib] Eliminate intermediate rounding error in floating-point strides to [stdlib] Eliminate intermediate rounding error in floating-point strides and related gardening Nov 25, 2017

@xwu xwu changed the title from [stdlib] Eliminate intermediate rounding error in floating-point strides and related gardening to [stdlib] Eliminate intermediate rounding error in floating-point strides (and related gardening) Nov 25, 2017

@xwu

This comment has been minimized.

Collaborator

xwu commented Nov 25, 2017

If @moiseev approves, this would be ready for a full test and potential merging.

let distance = _start.distance(to: _end)
return distance == 0 || (distance < 0) == (_stride < 0)
? ClosedRangeIndex(0)
: ClosedRangeIndex()

This comment has been minimized.

@moiseev

moiseev Nov 27, 2017

Member

I think we should just disallow creating strides that don't make sense. StrideThrough cannot be empty and that should be enforced in the initializer. Also the (distance < 0) == (_stride < 0) check, I think, should be performed in the initializer. These ill-formed sequences/collections don't seem useful to me.

This comment has been minimized.

@xwu

xwu Nov 27, 2017

Collaborator

If I had to design it from scratch, I absolutely agree. But it’d be source-breaking and there’s nothing that’s really broken by continuing to support it.

This comment has been minimized.

@moiseev

moiseev Nov 28, 2017

Member

Source breaking? Nah... it's just a runtime breakage.
But seriously, if anyone is using the empty stride-through or a mis-directed one, their code is already broken.
stride(from:1, through:0, by:1) should be similar to 1...0 and it's not.
Let me try to introduce the checks and run the compat test suite.
UPD: On a second thought, running the compat test suite will not give us any valuable information, precisely because it will be a runtime check.

This comment has been minimized.

@natecook1000

natecook1000 Nov 28, 2017

Member

I would be wary of adding a new precondition for the stride functions, since we've never had start <= end documented as a requirement. I wouldn't at all be surprised if people have switched from something like n...count to stride(from: n, through: count, by: 1) specifically to avoid having to check for n > count.

This comment has been minimized.

@xwu

xwu Nov 28, 2017

Collaborator

Precisely.

This comment has been minimized.

@moiseev

moiseev Nov 29, 2017

Member

I'm not saying we should just do it. It should go through evolution and such. It feels wrong to me allowing incorrect ranges in one place but not the other. As for the check, it will happen either way, if not explicitly in the user code, then in the iterator, that will just return nils all the time.

@moiseev

This comment has been minimized.

Member

moiseev commented Nov 27, 2017

Thanks @xwu ! Have you tried to make StrideTo conditionally conform to Collection instead?

@moiseev

This comment has been minimized.

Member

moiseev commented Nov 27, 2017

@swift-ci Please test

@swift-ci

This comment has been minimized.

Contributor

swift-ci commented Nov 27, 2017

Build failed
Swift Test Linux Platform
Git Sha - 76a2891571bb18ddc7f2f2fdf3eb935ba8b11958

@swift-ci

This comment has been minimized.

Contributor

swift-ci commented Nov 27, 2017

Build failed
Swift Test OS X Platform
Git Sha - 76a2891571bb18ddc7f2f2fdf3eb935ba8b11958

@xwu

This comment has been minimized.

Collaborator

xwu commented Nov 27, 2017

@moiseev Not Collection only: I want my RandomAccessSlice :)

But I did try Collection explicitly, followed immediately by BidirectionalCollection and RandomAccessCollection, but no dice. I’m sure this will be sorted out before long.

@moiseev

This comment has been minimized.

Member

moiseev commented Nov 28, 2017

@swift-ci Please Test Source Compatibility

1 similar comment
@moiseev

This comment has been minimized.

Member

moiseev commented Nov 29, 2017

@swift-ci Please Test Source Compatibility

@xwu

This comment has been minimized.

Collaborator

xwu commented Nov 29, 2017

Compatibility errors seem spurious. Not sure why two projects that never call 'stride' would fail.

@moiseev

This comment has been minimized.

Member

moiseev commented Nov 29, 2017

Yeah. These projects have been xfailed recently.

@moiseev moiseev merged commit 66f8a9b into apple:master Nov 29, 2017

4 of 5 checks passed

Swift Source Compatibility Suite on macOS Platform Build finished.
Details
Swift Test Linux Platform 10576 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 53000 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details

@xwu xwu deleted the xwu:fused-multiply-add-stride branch Dec 1, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment