Messages in this thread | | | Date | Tue, 06 Jun 2006 00:28:17 +0200 | From | Dag Arne Osvik <> | Subject | Re: [PATCH 4/4] Twofish cipher - x86_64 assembler |
| |
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |