regression for 3.1: bad sign extension?

[Geoff Keating]

> Date: Thu, 14 Mar 2002 17:11:44 -0800
> From: Bob Wilson <bwilson@tensilica.com>
> Organization: Tensilica, Inc.
> User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:0.9.9) Gecko/20020310
> X-Accept-Language: en-us, en
> CC: Geoff Keating <geoffk@geoffk.org>
> X-OriginalArrivalTime: 15 Mar 2002 01:11:45.0383 (UTC) FILETIME=[5FC18F70:01C1CBBE]
> 
> 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".

Yes, this is certainly incorrect.  '(set (subreg:SI (reg:HI 54) 0)'
implies 'clobber the high 16 bits of reg 54', and we don't want to
clobber them, we want to set them to 0.  [This assumes your words are
>= 32 bits wide.]  It would be worthwhile checking that
expand_compound_operation is doing the shifts in SImode not HImode.

> 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?

Yes, the right thing is to have the equivalent of:

(set (reg:SI 56) (const_int 0x00008000))

if that's cheaper than the original code.

-- 
- Geoffrey Keating <geoffk@geoffk.org> <geoffk@redhat.com>