Go: cmd/compile: intrinsify more of math/bits on amd64

Created on 18 Oct 2018 · 6Comments · Source: golang/go

Now that more API has been added to math/bits (#24813), this is a tracking bug to intrinsify more of it in the compiler for amd64.

As @randall77 said in https://github.com/golang/go/issues/24813#issuecomment-430817068 ...

Only Mul has been intrinsified so far.
I think we want Add/Sub intrinsified also (or better, recognize the Go code and generate the add-with-carry instruction).

FrozenDueToAge NeedsFix Performance help wanted

Source

bradfitz

😕1 👍1

Most helpful comment

I originally had added the Div64 intrinsic for amd64, but there were concerns during review about the behavior when the quotient won't fit into 64-bits. In particular, the pure go implementation ported from math/big returns a value of 1<<64 - 1, 1<<64 - 1 in this case.

if hi >= y {
    return 1<<64 - 1, 1<<64 - 1
}

whereas the DIVQ instruction will cause a divide by zero exception. Additionally, the 32-bit pure go version just truncates quo to 32-bits, which is not consistent either.

quo, rem = uint32(z/uint64(y)), uint32(z%uint64(y))

Currently, the documentation from the original proposal states:

hi must be < y otherwise the behavior is undefined (the quotient won't fit into quo).

The concern was that "undefined" shouldn't encompass both returning a value in one implementation and panic in another.

I propose that the behavior when hi >= y should be well-defined so that an intrinsic can be implemented consistently across platforms; either to panic or return a sensible value. I recommend changing the definition to panic when hi >= y since returning a value when the actual result does not fit in the output variable could be the source of subtle bugs. A panic will force users to handle hi >= y correctly if that is a possibility in their code. It also aligns with the behavior of the DIVQ instruction on amd64.

@griesemer @randall77

bmkessler on 19 Oct 2018

👍4

All 6 comments

if hi >= y {
    return 1<<64 - 1, 1<<64 - 1
}