Skip to content
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

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

Closed
jarifibrahim opened this issue Apr 19, 2019 · 9 comments
Closed

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

jarifibrahim opened this issue Apr 19, 2019 · 9 comments

Comments

@jarifibrahim
Copy link

# 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
Copy link

+1
met the same problem

@dgryski
Copy link
Owner

dgryski commented Apr 22, 2019

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
Copy link

@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
Copy link
Owner

dgryski commented Apr 22, 2019

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
Copy link

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
Copy link
Owner

dgryski commented Apr 22, 2019

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

@mmcloughlin
Copy link

mmcloughlin commented Apr 22, 2019

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.

dgryski added a commit that referenced this issue Apr 23, 2019
Not sure if it makes a difference in speed, but it breaks older Go
versions.

Fixes #17
@mmcloughlin
Copy link

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

@dgryski
Copy link
Owner

dgryski commented Apr 23, 2019

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.

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

No branches or pull requests

4 participants