On 07/09/15 23:10, Michael Matz wrote:
> Hi,
> 
> On Mon, 7 Sep 2015, Kugan wrote:
> 
>> For the following testcase (compiling with -O1; -O2 works fine), we have
>> a stmt with stm_code SSA_NAME (_7 = _ 6) and for which _6 is defined by
>> a GIMPLE_CALL. In this case, we are using wrong SUNREG promoted mode
>> resulting in wrong code.
> 
> And why is that?
> 
>> Simple SSA_NAME copes are generally optimized
>> but when they are not, we can end up using the wrong promoted mode.
>> Attached patch fixes when we have one copy.
> 
> I think it's the wrong place to fixing up.  Where does the wrong use come 
> from?  At that place it should be fixed, not after the fact.
> 
>>   _6 = bar5 (-10);
>>   ...
>>   _7 = _6;
>>   _3 = (long unsigned int) _6;
>>   ...
>>   if (_3 != l5.0_4)
> 
> There is no use of '_7' in this snippet so I don't see the relevance of 
> SUBREG_PROMOTED_MODE on it.
> 
> But whatever you do, please make sure you include the testcase for the 
> problem as a regression test:
> 

Thanks for the review.

This happens in ARM where definition of PROMOTED_MODE also changes the
sign. I am attaching the cfgdump for the test-case. This is part of the
existing test-case thats why I didn't include it as part of this patch.

for ;; _7 = _6;

(subreg:HI (reg:SI 113) 0)
 <ssa_name 0x7fd672c3ad38
    type <integer_type 0x7fd672c36540 short int HI
        size <integer_cst 0x7fd672c430c0 constant 16>
        unit size <integer_cst 0x7fd672c430d8 constant 2>
        align 16 symtab 0 alias set -1 canonical type 0x7fd672c36540
precision 16 min <integer_cst 0x7fd672c43078 -32768> max <integer_cst
0x7fd672c43090 32767>>
   def_stmt _7 = _6;

    version 7>
decl_rtl -> (reg:SI 113)
temp -> (subreg:HI (reg:SI 113) 0)
Unsignedp = 1

and we expand it to:

;; _7 = _6;

(insn 10 9 0 (set (reg:SI 113)
        (zero_extend:SI (subreg/u:HI (reg:SI 113) 0))) -1
     (nil))

but:

short int _6;
short int _7;

insn 10 above is wrong. _6 is defined by a call and therefore the sign
change in promoted mode is not true.

We should probably rearrange/or add a copy propagation to remove this
unnecessary copy but still this looks wrong to me.

Thanks,
Kugan