Clean up command line options for intgemm.

[ugermann]

If I understand correctly, there are currently there are currently 4 interconnected boolean command line options relating to intgemm

--optimize
--optmize8
--intgemm-shift
--intgemm-shift-all

This does not make sense to me as options are either mutually exclusive (things can't be 8-bit and 16-bit at the same time), or imply one another, e.g. intgemm-shift implies intgemm-shift and intgemm-shift implies optimize8.

These four options should be combined into a single string-valued option --optimize with the values

• none or no (default; regular float computation)
• intgemm16 (implicit, so that --optimize works as before)
• intgemm8
• intgemm8-shift
• intgemm8-shift-all

which are then interpreted in cpu::Backed::configureDevice() in tensor/cpu/backend.h

This should happen sooner rather than later so that we don't have command line interface legacy issues. Easy to fix now, a nightmare to change later once people have started using these options.

cc: @kpu

[XapaJIaMnu]

@ugermann , you are also missing --use-precomputed-alphas

I have proposed a change to the interface in my pull request against master, which unfortunately is not getting any love. I can update this with the proposed changes over the next few days. It's something like --gemm-precision int8/int16/int8shift/int8shiftalpha/int8shiftprecomputedalpha

[ugermann]

You may want to include in that change, if you haven't done so yet, the new Backend::configureDevice() that does device configuration in a single location rather than 3 copies of the same* code block (4 including the server's translation worker, which does not use the Translator class).

[ugermann]

@XapaJIaMnu

you are also missing --use-precomputed-alphas

Missing where? In Backend::configureDevice()?

[XapaJIaMnu]

@ugermann is this approach acceptable to you:

https://github.com/marian-nmt/marian-dev/blob/intgemm_reintegrated/src/tensors/backend.h#L32

If so, I'll go ahead and merge (including the additional options specific to this branch, such as --use-precomputed-alphas).

Cheers,

Nick

[ugermann]

1. As I mentioned earlier, I already implemented a Backend::configureDevice() member function in the current mt backend branch.
   I'd greatly prefer if all device configuration happened in that one function and not in four different locations in the code base.

2. To make the backend branch easier to find in the marian code base, I've created a new branch named mts-backend-stable. We will use that branch for the time being as the official stable branch for the Marian server. At some point, there will probably also be a mt-backend-dev branch.

3. Removing --optimize from the options will probably lead to backlash when trying to convince the Keepers of the Code to merge, because it may break exisiting scripts/regression tests, etc. The solution that I proposed preserves that backwards compatibility.

4. --int8 and --int16 in your proposal are still mutually exclusive and should therefore not be boolean flags.

Suggestion:
Replace original

  cli.add<bool>("--optimize",
      "Optimize speed aggressively sacrificing memory or precision");

With

  cli.add<std::string>("--optimize",
      "Optimize speed aggressively sacrificing memory or precision using intgemm (CPU only); values 'none', 'int8', 'int16'",
      "none")->implicit_val("int16");

and remove the "int8" and "int16" options.

[XapaJIaMnu]

Hey Uli,

Would this be acceptable? Here's what i coded. marian-nmt/marian-dev@22842e1 I don't like the number of permutations of intgemm options, so I'm welcome to suggestions.

Cheers,

Nick

[ugermann]

Note to the reader of this thread: conversation continues at marian-nmt/marian-dev@22842e1.

[XapaJIaMnu]

@ugermann, now maybe. I changed it a bit so that it's more similar to what might get into master one day. Also, we need to update all of our scripts.

[ugermann]

I'm lost. Can you point me to the relevant branch/commit? And I'd propose to change 'now maybe' to 'after the review'.

…

On Mon, Jul 20, 2020 at 7:51 PM Nikolay Bogoychev ***@***.***> wrote: @ugermann <https://github.com/ugermann>, now maybe. I changed it a bit so that it's more similar to what might get into master one day. Also, we need to update all of our scripts. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABCAWO3IYH2YPZJNPR6GZVTR4SG2HANCNFSM4OP3HTUQ> .

-- Ulrich Germann Senior Researcher School of Informatics University of Edinburgh

[XapaJIaMnu]

This branch: https://github.com/marian-nmt/marian-dev/compare/cmdparsing

It can definitely wait for after the review. There are some other fixes that should land from the intgemm_reintegrated_computstats branch... whne i code them.