gonum / internal

Internal routines for the gonum project [DEPRECATED]
21 stars 9 forks source link

asm/f64: Updated axpy assembly to wide, pipelined loops. #54

Closed Kunde21 closed 7 years ago

Kunde21 commented 7 years ago

Continuing gonum/internal#52 because the floor is lava.

@kortschak I addressed your comments from the review.

kortschak commented 7 years ago

In general, I'd suggest that you avoid remerging master into your work branches, it leads to the kind of problems we saw earlier. I have a workflow that involves a set of targetted commits for a PR and then gets follow-up commits from either me noticing things or PR comments. These follow-ups are labelled carefully so that I know which target they belong to and are then squashed into there target with an interactive rebase. I also alway work from the HEAD of master (rebasing onto master if necessary). This makes things much easier in the long run.

Kunde21 commented 7 years ago

I'm not sure what's going on with a96844f now. The diff is showing the newer code as old code.

kortschak commented 7 years ago

I know this is horrible, but would you be able to rebase your work onto master so we can work from a clean start. Then push -f to your branch.

Kunde21 commented 7 years ago

There. That makes more sense, now.

kortschak commented 7 years ago

OK. Now, if you do the following, we get back the linear set of commits with no empty merge commits.

git checkout f64Axpy
git fetch https://github.com/gonum/internal.git
git rebase FETCH_HEAD
git push -f origin f64Axpy # I am trusting you point origin at your own repo.

I just tried the equivalent locally and this works, but if it doesn't, you can always salvage by git reset --hard 1c227866cea4e5015ebf6930437e8447e78d8536.

kortschak commented 7 years ago

I'll look at c2572119baa80a91424fd959a7fcca5a16577be0 since it looks a little different to the last time I reviewed that set of changes - I could get to that in the next few days. The rest is good. You could do an interactive rebase like this:

pick 649fc44 asm/f64: Updated Axpy benchmarks with cleaner go1.7 constructs.
pick 7af3e4d asm/f64: Added and updated testing helper functions.
squash 485e106 asm/f64:  Benchmark updates for PR comments.
squash 3ea382d asm/f64:  Renamed gs placeholder to benchSink.
pick aee6591 asm/f64: Defined macros for named variable registers in axpy* code.
pick c257211 asm/f64:  Cleaned up axpy* assembly code.
vladimir-ch commented 7 years ago

I'll try.

kortschak commented 7 years ago

@Kunde21 because these reviews take such a long time, can we make this the last PR against github.com/gonum/internal and port the remaining four to github.com/gonum/gonum when it becomes open for changes? At the moment, these PRs are what is blocking the merging of repos.

Kunde21 commented 7 years ago

@kortschak That's not a problem for me. I'd like to get this one and Scal in before the merge, but if this is the only one, so be it.

I can always work around the merge schedule/needs.

Kunde21 commented 7 years ago

I squashed the fixes to clean up the log, but I left the L1Norm fix as a separate commit. It's ready to merge if that's not an issue.

kortschak commented 7 years ago

That all looks good to go. Thanks.