supranational / blst

Multilingual BLS12-381 signature library
Apache License 2.0
458 stars 175 forks source link

asm/*-x86_64.pl: disambiguate local labels. #165

Closed dot-asm closed 1 year ago

dot-asm commented 1 year ago

Another intermediate stepstone. Compile portable. Ping @jtraglia.

jtraglia commented 1 year ago

Neat, seems like a reasonable implementation so far 👍 I considered using __attribute__ ((constructor)) in my POC too, but it felt less portable since it's only supported by gcc/clang only & the Windows .CRT$XCU trick seemed a bit hacky. But I think it's perfectly fine.

Also, I think it would be good to extend blst_cpuid a little bit now to also check for Intel SHA Extensions (func=7, sub=0, info[1]>>29) and SSSE3 (func=1, sub=0, info[2]>>9) capabilities. Right now, it seems that blst_platform_cap is treated like a boolean. This will require fewer changes later when you need to detect features for sha256.

And just to verify, is the long-term goal to completely remove the preprocessor checks, like __ADX__?

dot-asm commented 1 year ago

it seems that blst_platform_cap is treated like a boolean.

Where? :-) :-) :-) But on a more serious note, why test instruction and not cmp?

[what's] the long-term goal [here]

We haven't discussed it yet...

dot-asm commented 1 year ago

I considered using __attribute__ ((constructor)) in my POC too, but it felt less portable

The rationale is that if a compiler doesn't support it, then it's an unsupported configuration:-) But on a serious note, formally speaking it's unreasonable to declare that everything is supported, it's just impossible, and consequently you're in your right to declare something as unsupported. But the setup is "stable", in the sense if a [hypothetical] compiler is unsupported, then it's not actually catastrophic.

dot-asm commented 1 year ago

is the long-term goal to completely remove the preprocessor checks, like __ADX__?

The dilemma is that __ADX__ code path is the one that was validated, hence there should be a way to guarantee that it's compiled without an option to bypass it. This is why blst "aggressively" pushes for it...

dot-asm commented 1 year ago

I've merged this. For now it changes effectively nothing in respect to the defaults, and the suggestion is to conduct the discussion about defaults elsewhere.