On Wed, 30 May 2007, Linus Torvalds wrote: > On Wed, 30 May 2007, Nicolas Pitre wrote: > > > > This patch adds a range test to hexval() in order to prevent this. Also > > let's index the hexval_table array directly in get_sha1_hex() using > > explicitly unsigned chars to avoid the range test producing faster > > code. > > Please just make hexval() take a "unsigned char" instead, solving the > problem in a manner that makes it much easier for the compiler to realize > that it never needs to sign-extend the value or test the end result.. Well, I did it the way I did for two reasons, the first being that random hexval() usage won't mask a bad value if it is passed an int which happens to be out of range (think EOF, or better yet -208, -207, etc). Yet gcc appears to be smart enough to skip the test if it is passed a value that cannot exceed the test range, like an unsigned char. > Ie I think your patch would be better off something like the following > instead (it would be a one-liner, but I agree that marking hexval_table > "const" is also a good idea). > > With this, gcc can just generate: > > movzbl (%rdi), %eax > movsbl hexval_table(%rax),%edx > movzbl 1(%rdi), %eax > movsbl hexval_table(%rax),%eax > sall $4, %edx > orl %eax, %edx > > for the code to generate a byte from two hex characters. My patch already produces code that looks like that for get_sha1_hex() on i386. But my second reason for the patch is that on ARM it should becomes: /* r4 = hexval_table, ip = hex */ ldrb r1, [ip], #1 ldrb r2, [ip], #1 ldrsb r0, [r4, r1] ldrsb r3, [r4, r2] orr r0, r3, r0, asl #4 I.e. the compiler has a greater chance to fold the post increment with the load byte instruction as above without the need for an extra add instruction. Nicolas - To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
