Re: regression for 3.1: bad sign extension?

[Bob Wilson <bwilson at tensilica dot com>]

I think I need some help to figure out where this is going wrong.  Here 
is what happens:

* The constant integer 0x8000 is used in an unsigned HImode comparison. 
  As described in my previous message, in the 3.1 branch the constant is 
  sign-extended when constructing the CONST_INT.

* The prepare_cmp_insn() function calls force_reg(), when 
preserve_subexpressions_p() is true, to put the CONST_INT into a 
register.  The sign-extended constant 0xffff8000 is put into the 
constant pool as an SImode value (because Xtensa only supports SImode 
constant pool values) and loaded into a register.  The register is then 
zero-extended before being used in the comparison.  The RTL is:

(insn 56 53 57 (set (subreg:SI (reg:HI 54) 0)
         (mem/u/f:SI (symbol_ref/u:SI ("*.LC0")) [6 S4 A32])) -1 (nil)
     (nil))

(insn 57 56 58 (set (reg:SI 55)
         (zero_extend:SI (reg:HI 53))) -1 (nil)
     (nil))

(insn 58 57 59 (set (reg:SI 56)
         (zero_extend:SI (reg:HI 54))) -1 (nil)
     (nil))

(jump_insn 59 58 62 (set (pc)
         (if_then_else (ne (reg:SI 55)
                 (reg:SI 56))
             (label_ref 65)
             (pc))) -1 (nil)
     (nil))

AFAIK, this is correct so far.  Note that when 
preserve_subexpressions_p() is false, the zero-extend is folded into the 
value put into the constant pool and there is no explicit zero-extend 
for the constant.

* The combiner removes insn 58 and replaces the use of reg 56 with 
"(subreg:SI (reg:HI 54) 0))".  The zero-extend is thus removed and value 
used in the comparison is 0xffff8000, which is wrong.  I traced through 
the combiner code that removes the zero-extend.  Here is a stack trace 
showing where it happens:

#0  expand_compound_operation (x=0x401605f8) at ../../cvs/gcc/combine.c:5551
#1  0x8187ef0 in combine_simplify_rtx (x=0x401605f8, op0_mode=HImode, 
last=0,
     in_dest=0) at ../../cvs/gcc/combine.c:4507
#2  0x8186a7d in subst (x=0x401605f8, from=0x40016000, to=0x40016000,
     in_dest=0, unique_copy=0) at ../../cvs/gcc/combine.c:3566
#3  0x8184090 in try_combine (i3=0x40159bc0, i2=0x4015c540, i1=0x0,
     new_direct_jump_p=0xbffff3fc) at ../../cvs/gcc/combine.c:1894

The comment in try_combine here is:

/* It is possible that the source of I2 or I1 may be performing an 
unneeded operation, such as a ZERO_EXTEND of something that is known to 
have the high part zero.  Handle that case by letting subst look at the 
innermost one of them....
*/

Roughly the expand_compound_operation first generates an ASHIFT by 16 
and then an LSHIFTRT by 16 and simplify_shift_const reduces the whole 
thing down to "(subreg:SI (reg:HI 54) 0))".  I'm not sure if this is 
correct or not.  It certainly doesn't fit the description in the comment 
of "something that is known to have the high parts zero".

There are really two issues here:

1) If the value in the constant pool is sign extended, then an explicit 
zero-extend is required somewhere.  It's currently being removed.

2) Using an explicit zero-extend here is poor code generation.  From my 
point of view, it would be easier to avoid the sign-extension in the 
first place.  Geoff's previous comment suggests that is not the right 
thing.  Perhaps I should change the combiner to detect a zero-extend of 
a value loaded from the constant pool and combine those two ops by 
putting a new zero-extended value in the pool.  Would that be a good 
solution?  Any better ideas?

Thanks as always for your help!


Bob Wilson wrote:
> OK, that makes sense....  It implies that (1) the real bug in this case 
> occurs when the subsequent zero-extend is incorrectly removed during the 
> combine phase, and that (2) I'm going to have to do some work to get GCC 
> to avoid explicit zero-extend operations, which are relatively expensive 
> on some Xtensa processors.
> 
> Thanks for pointing me in the right direction here, Geoff.
> 
> Geoff Keating wrote:
> 
>> Bob Wilson <bwilson@tensilica.com> writes:
>>
>>
>>> The problem occurs when there is an unsigned short (16-bits, HI mode)
>>> that gets (incorrectly?) sign-extended by this code.
>>
>>
>>
>> There's no such thing as an 'unsigned short' in RTL.  All HImode
>> values are the same, and there is only one way to write a HImode constant
>> of 0xffff: (const_int -1).
> 
> 


-- 
Bob Wilson                      Tel: (408) 327-7312
Tensilica, Inc.                 Fax: (408) 986-8919
3255-6 Scott Blvd.              email: bwilson@tensilica.com
Santa Clara, CA 95054