dgryski / go-farm

go-farm: a pure-Go farmhash implementation
MIT License
238 stars 22 forks source link

unrecognized instruction "IMUL3L" with go1.9/1.10 #17

Closed jarifibrahim closed 5 years ago

jarifibrahim commented 5 years ago
# github.com/dgryski/go-farm
../../dgryski/go-farm/fp_amd64.s:455: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:457: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:462: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:464: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:502: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:504: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:509: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:511: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:516: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:518: unrecognized instruction "IMUL3L"
../../dgryski/go-farm/fp_amd64.s:557: unrecognized instruction "IMUL3L"
asm: too many errors

See https://travis-ci.org/dgraph-io/badger/jobs/520608511 for more information

chennqqi commented 5 years ago

+1 met the same problem

dgryski commented 5 years ago

In the short term, downgrade the version of go-farm or upgrade your version of Go. I dont have an easy fix that maintains assembly for old versions by default.

mmcloughlin commented 5 years ago

@dgryski Would you consider an approach such as this?

// +build ignore

package main

import (
    "flag"

    . "github.com/mmcloughlin/avo/build"
    . "github.com/mmcloughlin/avo/operand"
    . "github.com/mmcloughlin/avo/reg"
)

var go111 = flag.Bool("go1.11", true, "generate for go 1.11 or later")

func imul3l(m uint32, x, y Register) {
    if *go111 {
        IMUL3L(U32(m), x, y)
    } else {
        t := GP32()
        MOVL(U32(m), t)
        IMULL(t, x)
        MOVL(x, y)
    }
}

func main() {
    flag.Parse()
    if *go111 {
        ConstraintExpr("go1.11")
    } else {
        ConstraintExpr("!go1.11")
    }

    TEXT("Mul7", NOSPLIT, "func(x uint32) uint32")
    Doc("Mul7 returns 7*x.")
    x := Load(Param("x"), GP32())
    imul3l(7, x, x)
    Store(x, ReturnIndex(0))
    RET()

    Generate()
}
dgryski commented 5 years ago

This would mean people who wanted support for older Go versions would need to generate their own assembly code. I'm fine with that. I'll come up with a patch tonight. Of course it's nicer to have everything work out-of-the-box, but I don't think we can do that here without a bunch of fiddly work.

mmcloughlin commented 5 years ago

This would mean people who wanted support for older Go versions would need to generate their own assembly code. I'm fine with that. I'll come up with a patch tonight. Of course it's nicer to have everything work out-of-the-box, but I don't think we can do that here without a bunch of fiddly work.

I was thinking that the repo would have two versions of the assembly code.

//go:generate go run asm -go1.11=true -out fp_amd64.go
//go:generate go run asm -go1.11=false -out fp_legacy_amd64.go

They could share a stub file (you'd need to write it yourself). I understand if you find this ugly, but I think it would work.

There's also the option of just downgrading to the multi-instruction form. How much of a hit would that actually be?

dgryski commented 5 years ago

I'd need to benchmark the slowdown. But that's probably a better approach.

mmcloughlin commented 5 years ago

I wouldn't be surprised if the performance hit was negligible, in which case that's surely the preferred solution.

Otherwise I don't think the go1.11 flag to the avo generator is that bad. The assembly is duplicated but since it's generated code, that doesn't matter much.

mmcloughlin commented 5 years ago

Nice. I think this actually exercises the "self-move pruning" mmcloughlin/avo#80.

dgryski commented 5 years ago

Yes, it cleaned up the self-MOVs in all the cases except the two places where the source and destination for the IMUL were different. Nicely done.