intel / isa-l_crypto

Other
267 stars 80 forks source link

Use embedded broadcast for avx512 constants. #139

Closed Shark64 closed 3 months ago

Shark64 commented 4 months ago

As asked in the closed pull request (#123) i've reworked the patch to just two commits: the first one is to replicate the constants using embedded broadcast for EVEX-coded instructions. This saves quite a lot of file size, especially for SHA512. The second patch is for SM3: basically i've tried to use VPTERNLOG wherever possibile, removing a couple of instructions from the critical path in each macro. On my system (Linux_x86-64, RocketLake CPU) `make test' passes all the tests without any errors, but I haven't tested it on other platforms.

pablodelara commented 4 months ago

Hi @Shark64. There is only one commit with all the changes proposed. Could you have multiple commits or better, multiple PRs to review this independently? Thanks! (also, the sign-off line should be at the end of the commit)

Shark64 commented 4 months ago

Hi, ok, you mean a different PR for each hashing function? I made just one commit because it's the same kind of change for all the hashes, but i can make a separate PR for each one if you prefer. Thanks!

pablodelara commented 4 months ago

No need to do it for every hash. What I meant is that you are also doing more than using broadcasts. For instance, in SM3, you are also adding ternary logic. You are also replacing some instructions with others or using different addressing modes.

Shark64 commented 3 months ago

Oh yeah now i understand what you meant. I did that because they seemed small enough changes that a different pull request and commit would clutter the git log; Do you prefer a separate pull request for them? I'll try to see if i can revert them in this branch, i'm not that expert with git wizardry but i'll give it a try ;)

pablodelara commented 3 months ago

Oh yeah now i understand what you meant. I did that because they seemed small enough changes that a different pull request and commit would clutter the git log; Do you prefer a separate pull request for them? I'll try to see if i can revert them in this branch, i'm not that expert with git wizardry but i'll give it a try ;)

Hi @Shark64. Right, "git add -p" is your friend :) You can rebase your branch on top of latest code and add your changes one by one, keeping only the ones for the broadcast first, create a commit and then same for the other changes. I'll submit comments on this.

We have pushed a fix for the clang-format, so no need for your other commit and we are going to push changes on sm3_mb, so I suggest waiting for this change (will be done shortly).

pablodelara commented 3 months ago

Hi @Shark64. We are pretty almost at code freeze now. OK to leave it for next release?

Shark64 commented 3 months ago

Hi @pablodelara, I was trying yesterday to rebase the code as you asked. Is the best way to just rebase my branch to the lastest code, save my patches somwhere else and then reapply them one by one? I ask because i ended up with having both my old commits and the new duplicate ones in the `git log', and i wasn't sure if it's the correct way of doing it. Thanks, sorry for the delay

pablodelara commented 3 months ago

Hi @pablodelara, I was trying yesterday to rebase the code as you asked. Is the best way to just rebase my branch to the lastest code, save my patches somwhere else and then reapply them one by one? I ask because i ended up with having both my old commits and the new duplicate ones in the `git log', and i wasn't sure if it's the correct way of doing it. Thanks, sorry for the delay

Yes, that's what I do. Apply them one by one and resolve the conflicts.

Shark64 commented 3 months ago

Yes, that's what I do. Apply them one by one and resolve the conflicts. Ok, I was wondering if there was some kind of git trick that i was missing ;) . I'll give it a try later today, just the changes to use embedded broadcast for avx512 in a single commit, right?

pablodelara commented 3 months ago

Yes, that's what I do. Apply them one by one and resolve the conflicts. Ok, I was wondering if there was some kind of git trick that i was missing ;) . I'll give it a try later today, just the changes to use embedded broadcast for avx512 in a single commit, right?

I am sure there are other tricks, but I don't know them :) Yes, those for now, thanks!

Shark64 commented 3 months ago

I made another branch with just the embedded broadcast changes, this way the git log looks less polluted to me. If it's ok with you I'll open another pull request to merge that?

pablodelara commented 3 months ago

Closing this PR, in favour of the new one.