Additional asm routines for gonum/internal performance

[Kunde21]

I'm creating this PR to allow for detailed comments on these suggested changes. It's the first set of asm routines to back gonum. They were created over multiple sessions, so there might be cleanup that I missed. Please add comments here or to any line/section that needs extra attention.

[kortschak]

There is a lot going on here. Would it be possible to squash/break this into a set of coherent commits that are reviewable independently.

I would suggest package splitting come first and then additions with tests for each function that is added (or tests added for functions already existing).

[Kunde21]

@kortschak I'll try to make it more organized, but a logical squash pattern might not be possible. I should have made a PR at the point of reorganization to minimize the size of everything. In the end, it's probably more organized to look at the *.s files as opposed to the commits.

[Kunde21]

@kortschak I did some commit cleanup today. It should be a bit easier to dig through, now.

[kortschak]

I've got a couple more commits to go through and I'd like to look over the ASM again more carfully (and have e.g. @vladimir-ch take a look).

Sorry for any silly questions here.

[kortschak]

Thanks for doing the clean up, it's very helpful.

[kortschak]

The generation code will have to be updated - this will fail tests as it stands.

[Kunde21]

Do we want to remove the assembly generation? I'd like to clean up things like the increment code like I used here.

[kortschak]

Do we want to remove the assembly generation? I'd like to clean up things like the increment code like I used here.

I'm happy with the clean-up. It would be nice if we can factor out the commonalities and do generation if possible though - we can leave that for the future if it will be easier.

[Kunde21]

@kortschak @vladimir-ch - I'm going to freeze this PR here. Please let me know if you have any questions, suggestions, or issues.

Benchmarks are here with comparisons to bare for loops.

Adjustments to the existing asm/f64 asm functions will be in a different PR and I'll have a separate PR for asm/f32, asm/c64, and asm/c128 dot products. This one is already pretty big.

[kortschak]

I don't think I can review this per commit since it seems that some files are touched in more than one commit.

I'll have another go at reviewing by file.

[kortschak]

General comments at this stage.

The Go looks OK to me with exception of where I've commented and the extrapolations from those.

I'd really like to see more commentary in the ASM. At the moment, some is commented well wrt the minutiae, but I'd also like to see comments at the same level of indentation as the ASM code that explains the ASM at approximately the level of Go expressions/subexpressions (the code in .../pkg/math/sincos_amd64.a is a reasonable approximation of this).

[Kunde21]

Are ASM comments like this example what you're looking for? Keep in mind that asmfmt will put a blank line above comments like line 54, so it's not going match the formatting of math/sincos_amd64.s.

[kortschak]

Yes, that looks good to me. I'm not worried about asmfmt making the formatting not look like the sincos_amd.s example, the semantics are more important (that file predates asmfmt and I doubt it will ever be run on it anyway).

[Kunde21]

That should cover the code commenting.

[kortschak]

Thank you for doing that. I should be able to get a chance to look at this over the weekend.

[kortschak]

I've found a little time to look at the asm and it looks OK to me with this superficial review (hoping for other eyes).

Sorry for the pedantry, but it keeps the code base sanish and consistent - apply the changes throughout if I trailed off.

Thank you for putting all this work in!

[btracey]

I took a quick glance through this PR. It's not clear to me what you would like me to be reviewing. Obviously, ideally I would do all of it, but I have no experience coding in assembler. Should I be trying anyway? If not, what are you hoping for?

[Kunde21]

@btracey The tests are first priority for me, as I know not everyone is familiar with asm. You've already helped us catch a register error with your suggestion.

If you want to poke through the asm, I believe the comments are thorough enough for you to understand what's going on in the code. You don't need to if you don't want to, though.

[btracey]

I made some high-level comments on the one test, and I imagine those extend to other tests. In general it looks okay, but I think here the combinatorics aren't so bad and we can just test all combinations.

[kortschak]

What is the situation with this?

[Kunde21]

Other than the suggestion we add random tests, I believe everything has been addressed. Just looking for final okay and merge.

[kortschak]

Thanks. I'll make a concerted effort to go through this this week.

[kortschak]

The other thing that needs to happen is that the go generate code needs to be integrated into this (or removed if appropriate - which seem more likely given the changes here).

[Kunde21]

With the package refactor cleaned up, the rest of the commits seem clear to me. Is there a reason we would need to rebase/squash anything else?

[Kunde21]

That should take care of all the comments on this PR.

@vladimir-ch This PR did balloon pretty quickly. I've created branches on my fork for the other changes I have planned to limit the size of those PRs.

[kortschak]

Thank you again.