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

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

Dag Arne Osvik wrote:
> 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.

Sorry.. that zeroing was of course due to load_s only doing xor..

> 
> 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;\
          ^^^ change to mov
>         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;\
          ^^^ change to mov
>         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