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

feat: support assembly generation on the CPU that has no adx #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zkbitcoin
Copy link

@zkbitcoin zkbitcoin commented May 23, 2024

Description

This PR addresses the issue where unsupported ADX CPU instruction sets cause invalid operation exceptions during unit tests (see #412).

A new flag, --no_adx, is introduced to handle logic branching for CPUs that do not support ADX and for CPUs that do not follow the assembly functions.

The following functions have been modified:

  • rawMMul (Montgomery multiplication)
  • rawMSquare (Montgomery Square)
  • rawToMontgomery (from number to Montomery form conversion)
  • rawFromMontgomery (from Montomery form to number conversion)

rawMMul1 uses adx but this is out of scope because Tachyon doesn't depend on it.

@chokobole
Copy link
Contributor

chokobole commented May 23, 2024

  1. Could you follow our commit convention? For example, for the first commit, you need to the commit type, feat: add non adx version of Montgomery modular multiplication/squaring and to.

  2. The sentence of the first commit isn't complete.

  3. I think you can squash most of the commits into one, since they are created for a single purpose, which generates an assmebly code when adx flag is not supported.

@chokobole
Copy link
Contributor

rawMMul1 still uses adcx instruction, but you think it's fine because Tachyon doesn't use it?

c.op("sub","rsp", "" + n64 * 8);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this blank.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blank is needed to change number to a string is how ops expects it otherwise it does different logic with numbers

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I mean the blank line!

build_defs.bzl Outdated
Comment on lines 60 to 61
if no_adx != None:
if no_adx == True:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it the same condition with below?

Suggested change
if no_adx != None:
if no_adx == True:
if no_adx:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, changing

.gitignore Outdated
Comment on lines 72 to 74

.idea
.clwb
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take this change out of this commit into a different commit, something like chore: update .gitignore.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing

build_defs.bzl Outdated
@@ -9,7 +9,8 @@ def generate_prime_field(
element_h_out = None,
generic_c_out = None,
raw_generic_c_out = None,
arm64_s_out = None):
arm64_s_out = None,
no_adx = None):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
no_adx = None):
no_adx = False):

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing

fr_no_adx.asm Outdated Show resolved Hide resolved
@@ -62,7 +63,7 @@ async function buildField(q, name, hpp_out, element_hpp_out) {
if (runningAsScript) {
const fs = require("fs");
var argv = require("yargs")
.usage("Usage: $0 -q [primeNum] -n [name] -oc [out .c file] -oh [out .h file] -oa [out .asm file]")
.usage("Usage: $0 -q [primeNum] -n [name] -no_adx [no_adx] -oc [out .c file] -oh [out .h file] -oa [out .asm file]")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should add -- instead of -.

Suggested change
.usage("Usage: $0 -q [primeNum] -n [name] -no_adx [no_adx] -oc [out .c file] -oh [out .h file] -oa [out .asm file]")
.usage("Usage: $0 -q [primeNum] -n [name] --no_adx [no_adx] -oc [out .c file] -oh [out .h file] -oa [out .asm file]")

Please just test with this.

────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
modified: src/buildzqfield.js
────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
@ src/buildzqfield.js:92 @ if (runningAsScript) {
        .alias("n", "name")
        .argv;

+    console.log(argv);

    const q = bigInt(argv.q);

    const asmFileName =  (argv.oa) ? argv.oa : argv.name.toLowerCase() + ".asm";

This shows me this output

node src/buildzqfield.js -q 7 -n fq -noadx --oc a.c
{
  _: [],
  q: 7,
  prime: 7,
  n: [ 'fq', true ],
  name: [ 'fq', true ],
  o: true,
  a: true,
  d: true,
  x: true,
  oc: 'a.c',
  '$0': 'src/buildzqfield.js'
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, existing flags -oc, -oh and -oa also should be fixed to --oc, --oh and --oa. So I hope you add a separate commit for this change as well.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing


function buildFromMontgomery_no_adx(fn, q) {
return templateMontgomery_no_adx(fn, q, function(c, params, i, r0, r1, r2) {
const {t, n64, canOptimizeConsensys} = params;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const {t, n64, canOptimizeConsensys} = params;
const {n64} = params;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing


function buildSquare_no_adx(fn, q) {
return templateMontgomery_no_adx(fn, q, function(c, params, i, r0, r1, r2) {
const {t, n64, canOptimizeConsensys} = params;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const {t, n64, canOptimizeConsensys} = params;
const {n64} = params;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing


function buildMul_no_adx(fn, q) {
return templateMontgomery_no_adx(fn, q, function(c, params, i, r0, r1, r2) {
const {t, n64, canOptimizeConsensys} = params;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const {t, n64, canOptimizeConsensys} = params;
const {n64} = params;

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing

c.code.push("jnz " + fn + "_mulM_sq") // q is lower
}

c.code.push("; If equal substract q")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
c.code.push("; If equal substract q")
c.code.push("; If equal subtract q")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed bad spelling (safe replace) across other code as well

@zkbitcoin
Copy link
Author

  1. Could you follow our commit convention? For example, for the first commit, you need to the commit type, feat: add non adx version of Montgomery modular multiplication/squaring and to.
  2. The sentence of the first commit isn't complete.
  3. I think you can squash most of the commits into one, since they are created for a single purpose, which generates an assmebly code when adx flag is not supported.

rawMMul1 still uses adcx instruction, but you think it's fine because Tachyon doesn't use it?

correct change was to explicitly address tachyon compatibility , changing rawMMul1 is outside of this scope

build_defs.bzl Outdated Show resolved Hide resolved
@@ -311,6 +313,154 @@ function buildFromMontgomery(fn, q) {
});
}

// no adx assembler

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about removing the empty line here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed // no adx assembler a leftover , blanks condensed

c.op("xor","r8","r8");
c.op("xor","r9","r9");
c.op("xor","r10","r10");
// Main loop

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Main loop
// Main loop

How about adding an empty line here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@fakedev9999
Copy link

@zkbitcoin I also think that you could squash the first 4 commmits into one! Could you do that for me?

@zkbitcoin
Copy link
Author

zkbitcoin commented May 23, 2024

@zkbitcoin I also think that you could squash the first 4 commmits into one! Could you do that for me?

I can reverse fork, save project redo fork , do proper commit (all into 1) if you dont mind losing history here . sure can

@chokobole chokobole changed the title Support no adx feat: support assembly generation on the CPU that has no adx May 24, 2024
for (let i=0; i < n64 * 2; i++) {
setR(i);
round(c, params, i, r0, r1, r2);
for (let j = i - 1; j >= 0; j--) { // All ms
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does All ms mean?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all modulus (kept in q array) are being processed in this loop, comment does not add anything extra code in mul q explains it, will remove it

c.op("sub","rsp", "" + n64 * 8);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I mean the blank line!

@@ -92,7 +92,7 @@ if (runningAsScript) {
const q = bigInt(argv.q);

const asmFileName = (argv.oa) ? argv.oa : argv.name.toLowerCase() + ".asm";
const no_adx = (argv.no_adx) ? argv.no_adx : false;
const no_adx = (argv.no_adx) ? argv.no_adx : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. then isn't undefined value is more proper for this purpose?

add non adx version of Montgomery modular multiplication/squaring and to

fix stack allcoation and de-allocation

correct flag no_adx condition

fix no_adx flag propagation

add adx detection support via /proc/cpuinfo

perform detection only if flag was not explicitely specified
@zkbitcoin
Copy link
Author

@zkbitcoin I also think that you could squash the first 4 commmits into one! Could you do that for me?

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

Successfully merging this pull request may close these issues.

3 participants