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
-
To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
