[KJ][Patch] re-send of remove second masking instance of variable

[KJ][Patch] re-send of remove second masking instance of variable

[Darren Jenkins\]

[-- Attachment #1: Type: text/plain, Size: 1447 bytes --]

G'day list

This is a re-send of the last patch (that i stuffed up).
Because there are 3 instances in fpu_trig.c of an extra declaration of a
variable.

static void fyl2x(FPU_REG *st0_ptr, u_char st0_tag)

declares "u_char sign;" three times.
once at 1129, once at 1216, and once at 1271.


static void fyl2xp1(FPU_REG *st0_ptr, u_char st0_tag)

declares "u_char sign;" twice
1475: "u_char sign, sign1;"
and
1647: "u_char sign = getsign(st1_ptr);"

This Patch removes all three superfluous instances.




Signed-off-by: Darren Jenkins <darrenrjenkins@gmail.com> 

--- linux-2.6.16-rc2/arch/i386/math-emu/fpu_trig.c.orig	2006-02-10 21:33:37.000000000 +1100
+++ linux-2.6.16-rc2/arch/i386/math-emu/fpu_trig.c	2006-02-10 22:30:45.000000000 +1100
@@ -1213,7 +1213,6 @@ static void fyl2x(FPU_REG *st0_ptr, u_ch
 	    }
 	  else
 	    {
-	      u_char sign;
 	      sign = getsign(st1_ptr)^SIGN_NEG;
 	      if ( FPU_divide_by_zero(1, sign) < 0 )
 		return;
@@ -1268,7 +1267,7 @@ static void fyl2x(FPU_REG *st0_ptr, u_ch
 	}
       else
 	{
-	  u_char sign = getsign(st1_ptr);
+	  sign = getsign(st1_ptr);
 
 	  if ( (st1_tag == TW_Denormal) && (denormal_operand() < 0) )
 	    return;
@@ -1644,7 +1643,7 @@ static void fyl2xp1(FPU_REG *st0_ptr, u_
 	 80486 sensibly says that it is o.k. */
       else
 	{
-	  u_char sign = getsign(st1_ptr);
+	  sign = getsign(st1_ptr);
 	  FPU_copy_to_reg1(&CONST_INF, TAG_Special);
 	  setsign(st1_ptr, sign);
 	}



[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ][Patch] re-send of remove second masking instance of variable

[Matthew Wilcox]

[-- Attachment #1: Type: text/plain, Size: 1065 bytes --]

On Fri, Feb 10, 2006 at 11:01:18PM +1100, Darren Jenkins" wrote:
> This is a re-send of the last patch (that i stuffed up).
> Because there are 3 instances in fpu_trig.c of an extra declaration of a
> variable.

Do you ever compare the code before and after your changes?  I ask
because I don't think gcc does live range splitting yet.  So a variable
which may have been register-allocated before may now move to the stack.
It may often make more sense to rename a variable or manually do LRS.
Examples:

int foo;
if (x) {
	...
}

if (y) {
	int foo;
	...
}

transform either to (a):

if (x) {
	int foo;
	...
}

if (y) {
	int foo;
	...
}

or to (b):

int foo;
if (x) { ... }

if (y) {
	int bar;
	...
}

Option (c) [the one you use] can be better.  If foo is a stack allocated
variable anyway, this will force gcc to use the same stack slot for both.

So I'm not saying these patches are wrong, and I really want to see
shadow variables eliminated (we should always have -Wshadow enabled, IMO).
I'd just like to see some analysis of the consequences of each change.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [KJ][Patch] re-send of remove second masking instance of

[Darren Jenkins\]

[-- Attachment #1: Type: text/plain, Size: 3933 bytes --]

On Fri, 2006-02-10 at 05:41 -0700, Matthew Wilcox wrote:

> Do you ever compare the code before and after your changes?  I ask
> because I don't think gcc does live range splitting yet.  So a variable
> which may have been register-allocated before may now move to the stack.
> It may often make more sense to rename a variable or manually do LRS.

Looking at that file I think you might be right, perhaps the author was
trying to manually do LRS or something as in the next function there is

...
else if ()
{
	u_char sign =
}
else if ()
{
	u_char sign =
}

but as they have mutually exclusive scope here I think that it is ok to
do it like that? or perhaps I should change that too ?

> Option (c) [the one you use] can be better.  If foo is a stack allocated
> variable anyway, this will force gcc to use the same stack slot for both.
> 
> So I'm not saying these patches are wrong, and I really want to see
> shadow variables eliminated (we should always have -Wshadow enabled, IMO).
> I'd just like to see some analysis of the consequences of each change.
> 

Ok doing a objdump on both the .o files then doing a diff on the two
gives me this. 

Note: I _think_ I did this right, but no guarantees.

--- fpu_trig.S.orig	2006-02-11 13:32:09.000000000 +1100
+++ fpu_trig.S	2006-02-11 13:40:28.000000000 +1100
@@ -1,5 +1,5 @@
 
-fpu_trig.o.orig:     file format elf32-i386
+fpu_trig.o:     file format elf32-i386
 
 Disassembly of section .text:
 
@@ -725,7 +725,7 @@ Disassembly of section .text:
      872:	75 48                	jne    8bc <fyl2xp1+0x99>
      874:	8a 57 09             	mov    0x9(%edi),%dl
      877:	80 e2 80             	and    $0x80,%dl
-     87a:	88 55 d2             	mov    %dl,0xffffffd2(%ebp)
+     87a:	88 55 db             	mov    %dl,0xffffffdb(%ebp)
      87d:	8d 55 e8             	lea    0xffffffe8(%ebp),%edx
      880:	8a 46 09             	mov    0x9(%esi),%al
      883:	24 80                	and    $0x80,%al
@@ -738,7 +738,7 @@ Disassembly of section .text:
      896:	e8 fc ff ff ff       	call   897 <fyl2xp1+0x74>
      89b:	0f b6 55 d3          	movzbl 0xffffffd3(%ebp),%edx
      89f:	8d 4d e8             	lea    0xffffffe8(%ebp),%ecx
-     8a2:	0f b6 45 d2          	movzbl 0xffffffd2(%ebp),%eax
+     8a2:	0f b6 45 db          	movzbl 0xffffffdb(%ebp),%eax
      8a6:	56                   	push   %esi
      8a7:	57                   	push   %edi
      8a8:	e8 fc ff ff ff       	call   8a9 <fyl2xp1+0x86>
@@ -896,11 +896,11 @@ Disassembly of section .text:
      aa1:	8a 56 09             	mov    0x9(%esi),%dl
      aa4:	b8 00 00 00 00       	mov    $0x0,%eax
      aa9:	80 e2 80             	and    $0x80,%dl
-     aac:	88 55 db             	mov    %dl,0xffffffdb(%ebp)
+     aac:	88 55 d2             	mov    %dl,0xffffffd2(%ebp)
      aaf:	ba 02 00 00 00       	mov    $0x2,%edx
      ab4:	e8 fc ff ff ff       	call   ab5 <fyl2xp1+0x292>
      ab9:	8a 46 09             	mov    0x9(%esi),%al
-     abc:	80 7d db 00          	cmpb   $0x0,0xffffffdb(%ebp)
+     abc:	80 7d d2 00          	cmpb   $0x0,0xffffffd2(%ebp)
      ac0:	74 04                	je     ac6 <fyl2xp1+0x2a3>
      ac2:	0c 80                	or     $0x80,%al
      ac4:	eb 02                	jmp    ac8 <fyl2xp1+0x2a5>



Like I said I know almost nothing about x86 arch asm but it looks to me
like they use the same addressing mode, just grab it from a different
location in memory. Perhaps you can tell me exactly whats going on ?

Also I went back and looked at the C code for problems resulting from
inserting the intermediate "int sign" variable into the global scope and
clobbering the global "int sign" variable. There are places where the
global "int sign" is used after(in the source file) the intermediate
without an assignment first but they all appear (and I hope I got this
right) to be on paths excluding the intermediate "int sign" variable
use.

I hope this answers your questions

Darren J.


[-- Attachment #2: Type: text/plain, Size: 168 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
https://lists.osdl.org/mailman/listinfo/kernel-janitors