Rename to skein_block_asm.s to .S, to use Clang's integrated assembler
ClosedPublic
Actions

Authored by emaste on Nov 3 2016, 1:47 PM.

Details

Reviewers

allanjude
tsoome
cem
arichardson
jmg

Commits

rS361853: Rename skein_block_asm.s to .S and assemble using Clang IAS

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

emaste updated this revision to Diff 21971.Nov 3 2016, 1:47 PM

emaste retitled this revision from to Rename to skein_block_asm.s to .S, to use Clang's integrated assembler.

emaste updated this object.

emaste edited the test plan for this revision. (Show Details)

emaste added reviewers: allanjude, tsoome.

emaste added a subscriber: bdrewery.

Seems ok for me, assuming its tested and compiler driver does build it properly:)

This revision is now accepted and ready to land.Nov 3 2016, 2:11 PM

This doesn't actually build with the integrated assembler today -- see https://bugs.llvm.org/show_bug.cgi?id=33405

emaste mentioned this in D16844: Don't build skein_block_asm.s if we don't have an as binary.Aug 22 2018, 2:59 PM

rebase

emaste abandoned this revision.Jun 5 2020, 5:51 PM

emaste reclaimed this revision.

emaste added reviewers: cem, arichardson.

cem accepted this revision.Jun 5 2020, 5:55 PM

cem added inline comments.

lib/libmd/Makefile
122	I might keep the comment explaining the constant (if accurate)
126	Hm, did we not kill ripemd yet?
sys/modules/crypto/Makefile
35	Do we want these in the other make file?

This revision is now accepted and ready to land.Jun 5 2020, 5:55 PM

emaste added inline comments.Jun 5 2020, 5:56 PM

lib/libmd/Makefile
121	strip-local-absolute in LLVM: https://bugs.llvm.org/show_bug.cgi?id=27201

Looks good.

We assemble skein_block_asm.S four times - once for sys/modules/crypto, and three times for lib/libmd (.o, .pico, .po). All four instances have the same change when assembled with Clang IAS vs. GNU as 2.17.50:

│ │ ├── objdump --line-numbers --disassemble --demangle --reloc --section=.text {}
│ │ │ @@ -1884,17 +1884,17 @@
│ │ │      1b8f:        00
│ │ │      1b90:        48 89 bd a8 00 00 00    mov    %rdi,0xa8(%rbp)
│ │ │      1b97:        48 89 b5 b0 00 00 00    mov    %rsi,0xb0(%rbp)
│ │ │      1b9e:        48 89 95 b8 00 00 00    mov    %rdx,0xb8(%rbp)
│ │ │      1ba5:        48 89 8d c0 00 00 00    mov    %rcx,0xc0(%rbp)
│ │ │      1bac:        4c 8b 4f 18             mov    0x18(%rdi),%r9
│ │ │      1bb0:        eb 0e                   jmp    1bc0 <Skein1024_block_loop>
│ │ │ -    1bb2:        66 66 2e 0f 1f 84 00    data16 nopw %cs:0x0(%rax,%rax,1)
│ │ │ -    1bb9:        00 00 00 00
│ │ │ -    1bbd:        0f 1f 00                nopl   (%rax)
│ │ │ +    1bb2:        66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
│ │ │ +    1bb9:        00 00 00
│ │ │ +    1bbc:        0f 1f 40 00             nopl   0x0(%rax)
│ │ │
│ │ │  0000000000001bc0 <Skein1024_block_loop>:
│ │ │  Skein1024_block_loop():
│ │ │      1bc0:        4c 8b 47 10             mov    0x10(%rdi),%r8
│ │ │      1bc4:        4c 03 85 c0 00 00 00    add    0xc0(%rbp),%r8
│ │ │      1bcb:        4d 89 ca                mov    %r9,%r10
│ │ │      1bce:        4d 31 c2                xor    %r8,%r10

That is, for padding GNU as emits an 11 byte NOP followed by a 3 byte NOP, while IAS emits a 10 byte NOP followed by a 4 byte NOP.

Clang will accept -defsym SKEIN_LOOP=0 (single dash). But using the C preprocessor also seems fine to me.