Re: [PATCH 4/4] Twofish cipher - x86_64 assembler

[Dag Arne Osvik <da@osvik.no>]

Joachim Fritschi wrote:
> On Sunday 04 June 2006 23:01, Dag Arne Osvik wrote:
>> Andi Kleen wrote:
>>> On Sunday 04 June 2006 15:16, Joachim Fritschi wrote:
>>>> This patch adds the twofish x86_64 assembler routine.
>>>>
>>>> +/* Defining a few register aliases for better reading */
>>> Maybe you can read it now better, but for everybody else it is extremly
>>> confusing. It would be better if you just used the original register
>>> names.
>> I'd agree if you said this code could benefit from further readability
>> improvements.  But you're arguing against one.
>>
>> Too bad AMD kept the old register names when defining AMD64..
> 
> I'd agree that the original register names would only complicate things. 
> 
> Can you give me any hint what to improve or maybe provide a suggestion on how 
> to improve the overall readabilty.

It looks better on second reading, but I have some comments:

Remove load_s - it's needless and (slightly) confusing
There are some cases of missing ## D
Why semicolon after closing parenthesis in macro definitions?
Try to align operands in columns
It would be nice to have some explanation of macro parameter names

Btw, why do you keep zeroing tmp registers when you don't need to?
32-bit ops zero the top half of the destination register.

Here's an example of a modified macro (modulo linewrapping by my mail
client):

#define
encrypt_round(a,b,olda,oldb,newa,newb,ctx,round,tmp1,tmp2,key1,key2) \
        load_round_key(key1,key2,ctx,round);\
        movzx   a ## B,         newa ## D;\
        movzx   a ## H,         newb ## D;\
        ror     $16,            a    ## D;\
        xor     s0(ctx,newa,4), tmp1 ## D;\
        xor     s1(ctx,newb,4), tmp1 ## D;\
        movzx   a ## B,         newa ## D;\
        movzx   a ## H,         newb ## D;\
        xor     s2(ctx,newa,4), tmp1 ## D;\
        xor     s3(ctx,newb,4), tmp1 ## D;\
        ror     $16,            a    ## D;\
        movzx   b ## B,         newa ## D;\
        movzx   b ## H,         newb ## D;\
        ror     $16,            b    ## D;\
        xor     s1(ctx,newa,4), tmp2 ## D;\
        xor     s2(ctx,newb,4), tmp2 ## D;\
        movzx   b ## B,         newa ## D;\
        movzx   b ## H,         newb ## D;\
        xor     s3(ctx,newa,4), tmp2 ## D;\
        xor     s0(ctx,newb,4), tmp2 ## D;\
        ror     $15,            b    ## D;\
        add     tmp2 ## D,      tmp1 ## D;\
        add     tmp1 ## D,      tmp2 ## D;\
        add     tmp1 ## D,      key1 ## D;\
        add     tmp2 ## D,      key2 ## D;\
        mov     olda ## D,      newa ## D;\
        mov     oldb ## D,      newb ## D;\
        mov     a    ## D,      olda ## D;\
        mov     b    ## D,      oldb ## D;\
        xor     key1 ## D,      newa ## D;\
        xor     key2 ## D,      newb ## D;\
        ror     $1,             newa ## D

At least a little bit more readable, right?

-- 
  Dag Arne