Skip to content

Should SIMD add/sub/etc. trigger panics on overflow in debug mode? #56

@workingjubilee

Description

@workingjubilee
Member

Per topic, this affects our vector implementations of Add, Sub, and Neg.

This came up during discussion for #54 and #55.

Activity

added
I-nominatedWe should discuss this at the next weekly meeting
E-needs-designCall for participation: Needs design.
on Jan 27, 2021
Lokathor

Lokathor commented on Jan 27, 2021

@Lokathor
Contributor

I don't think so.

First of all a technical question: can rust code even cfg on overflow-checks being true or false?

Second: even if we could, we should not. it would make the debug code unacceptably slow. rust has a lot of things that make for slow debug code and long compiles in release, we should not compound the problem if we don't have to.

calebzulawski

calebzulawski commented on Jan 27, 2021

@calebzulawski
Member

I do tend to agree that maybe SIMD should default to wrapping implementations (but provide checked member fns as well). I would have left Neg alone in #54 if I hadn't misread it...

workingjubilee

workingjubilee commented on Jan 27, 2021

@workingjubilee
MemberAuthor

First of all a technical question: can rust code even cfg on overflow-checks being true or false?

I do not believe so.

Second: even if we could, we should not. it would make the debug code unacceptably slow. rust has a lot of things that make for slow debug code and long compiles in release, we should not compound the problem if we don't have to.

In the spirit of sounding out the alternatives (or at least warming up our counterarguments for when someone inevitably asks), I must note that scalar math triggers panics on overflows in debug mode. Why should this not be consistent? Yes, it is slower, but what special need is there to speed this up in debug mode versus any special need in scalar math?

Lokathor

Lokathor commented on Jan 27, 2021

@Lokathor
Contributor

"because you only pick simd in the first place if you want to go faster"

but I'm quite biased. I've never ever wanted checked in debug wrapping in release anyway. it was a bad decision then, we shouldn't double down on it now.

calebzulawski

calebzulawski commented on Jan 27, 2021

@calebzulawski
Member

I think I agree, the only reason to use SIMD is for performance, so the default should probably be the most performant. I tend to agree that wrapping would probably have been the better default. When safety is involved I use add_checked or whatever anyway. I'm probably biased in the same way.

thomcc

thomcc commented on Jan 27, 2021

@thomcc
Member

but I'm quite biased. I've never ever wanted checked in debug wrapping in release anyway. it was a bad decision then, we shouldn't double down on it now.

I don't agree with this, but I agree with the broad sentiment that it doesn't make sense for SIMD.

calebzulawski

calebzulawski commented on Jan 27, 2021

@calebzulawski
Member

I do think it's worth pointing out that there are generic bit tricks you can do to produce relatively performant "any of" mask checks (and special instructions like movemask on some architectures). The branch will really be what hurts, though, and that's unavoidable.

thomcc

thomcc commented on Jan 27, 2021

@thomcc
Member

I think a bigger issue is that it encourages some sort of manual checking at various points which is awkward and usually unnecessary.

I'm not concerned about debug performance here, short of wanting to avoid the debug performance issues of core::arch where nothing is inlined and you typically loose a ton of debug perf when switching from scalar to simd.

workingjubilee

workingjubilee commented on Jan 27, 2021

@workingjubilee
MemberAuthor

I would also note that if you are doing one math, you are likely to do another math, as it were, in SIMD, whereas ordinary math ops are much more likely to appear isolatively, and it is correct to note that any such guard should probably not be in the middle of the code! Also, SIMD ops being much more likely to come one after the other means that any branching in debug mode is much harder on perf since it will happen a lot, yet is much more likely to be a concern with a typical usage of SIMD stuff such as "draw a million triangles, now" or "encode/decode thousands of digital subsamples of audio, now".

So, "We are at least mildly against panicking on debug overflows because the perf concerns are much higher-pressure on testing SIMD code, and also because we think it results in an antipattern" seems like sound reasoning to me.

workingjubilee

workingjubilee commented on Feb 1, 2021

@workingjubilee
MemberAuthor

If we go with this, we should include a note to this effect in our documentation.

tarcieri

tarcieri commented on Feb 1, 2021

@tarcieri

Cryptography occasionally benefits from checked arithmetic, but offhand I can't think of a cryptographic SIMD use case which needs anything other than Wrapping semantics.

Also generally where checked arithmetic is appropriate in cryptographic use cases it's an "always on" sort of thing and not the overflow-checks style on-for-debug, off-for-release approach.

workingjubilee

workingjubilee commented on Feb 15, 2021

@workingjubilee
MemberAuthor

We discussed this on 2021-02-08 and came to a consensus that we should avoid branching in our API, generally, unless it makes sense to always check for a branch. Here it doesn't, really, and, well, it might be UB in certain languages but it certainly isn't that in Rust. And SIMD intrinsics for specific architectures (as far as we are aware) do not define overflow as UB either, since you're just compiling relatively "directly" to a set of assembly instructions, so we don't think it's exactly a familiar expectation for SIMD programmers. Obviously-ish that means that we're still having some equivalent of the checked_* math ops, since, well, it's in the name to check!

We further discussed this on 2021-02-15 and are even currently inclined against providing the wrapping_* ops, because their presence would imply you need to use them in order to avoid overflow in debug mode.

gilescope

gilescope commented on May 2, 2021

@gilescope

Could be worth pointing out in * ops docs like add that the operations wrap then?

added
E-needs-docsNeeds documentation added.
and removed
E-needs-designCall for participation: Needs design.
on May 3, 2021
added a commit that references this issue on Feb 9, 2022
6143bde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    E-needs-docsNeeds documentation added.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Participants

      @tarcieri@calebzulawski@gilescope@thomcc@Lokathor

      Issue actions

        Should SIMD add/sub/etc. trigger panics on overflow in debug mode? · Issue #56 · rust-lang/portable-simd