[patch RFA] SH: Fix build failure for sh64-*-linux-gnu

[Joern RENNECKE]

Kaz Kojima wrote:

>Joern RENNECKE <joern.rennecke@st.com> wrote:
>  
>
>>This is not actually specific for PIC.  Neither does it seem necessary...
>>    
>>
>[snip]
>  
>
>>... since you can you 's' for the constraint.  (In principle 'i' should 
>>work as well, but
>>we shouldn't be generating out-of-range numeric constants in the first 
>>place.)
>>
>>It also seems prudent to remove the generation of the sign_extend / 
>>truncate steps
>>from gen_movsi_const / gen_movdi_const / gen_movdi_const_32bit .
>>    
>>
Sorry, I haven't thought this through to its logical conclusion yesterday.
In order to avoid surprises from the optimizers, we should try to use 
canonical rtl
where feasible.  That not only means to avoid redundant truncates / 
extends, but
also to wrap constant expressions with a const - and not bury another 
const deep
inside.  I.e. it should be

(const:SI (zero_extend:SI (truncate:HI (unspec:SI ...))))

Likewise, if we have a numeric constant, it's not canonical to have a 16 
bit signed
constant and then force it unsigned with TRUNCATE / ZERO_EXTEND.
We should rather use an unsigned constant, with a suitable constraint, 
i.e. K16

#define CONST_OK_FOR_K16(VALUE) (((HOST_WIDE_INT)(VALUE))>= 0 \
                                 && ((HOST_WIDE_INT)(VALUE)) <= 65535)

AFAICS, this requires adjustments of three splitters:
- in movdi_const_16bit+1, low should not be sign-extended - only 
zero-extended
   (i.e. delete the two lines below "low &= 0xffff;")
- likewise for val in the following splitter ; that splitter also 
explicitly generates
   a zero_extend / truncate pair, which would have to be removed.
- in *double_shori, the calls to gen_int_mode should use SImode, and the 
second
  of these calls has to mask the first argument with & 65535 ;
  moreover, the split condition was wrong to start with, since shori is 
an unsigned
  operation.  It should be something like:
 "TARGET_SHMEDIA
   && ! (INTVAL (operands[2]) & ~(unsigned HOST_WIDE_INT) 0xffffffffUL)",
  although I don't see how we can avoid a warning for hosts with 32 bit
  HOST_WIDE_INT.


The coverage of C16 for literal constants also is limited to 
non-canonical rtl.
We shouldn't generate that non-canonical rtl in the first place.  Also, C16
describes a signed constant (we emit signed 16 bit symbolic constants as 
unsigned
because that's the way the assembler wants it, but we should make sure 
we don't mix
up signed/unsigned in the rtl lest the optimizers will have their revenge).
We used to wrap a C16 constant in a truncate/zero_extend to make it an 
unsigned
constant, but we really should have a separate constraint for an 
unsigned 16 bit
symbolic constant.

I propose:
    Css: signed 16 bit symbolic constant (signed short)
    Cus: unsigned 16 bit symbolic constant (unsigned short)
where Css is like C16, except for the comment, and Cus tests for ZERO_EXTEND
instead of SIGN_EXTEND.
mov[qhsd]i_media*  /  movv*i_i pattern movi alternatives then use
I16Css / I16CssZ constraints, and shori uses a K16Cus constraint.

The shori_media pattern then should be something like:
(define_insn "shori_media"
  [(set (match_operand:DI 0 "ext_dest_operand" "=r,r")
        (ior:DI (ashift:DI (match_operand:DI 1 "arith_reg_operand" "0,0")
                           (const_int 16))
                (match_operand:DI 2 "immediate_operand" "K16Cus,nF")))]

except I'm not sure if the nF alternative still has a purpose.

>I've attached the revised patch.  A few new insns are needed to remove
>extend/truncate steps form movdi_const.
>  
>
I don't like having that many patterns for shori.  With a Cus constraint 
as described
above they should not be necessary.
The outputting of the & 65535 and the shift is handled in the CONST case of
sh.c:print_operand .  The check for SIGN_EXTEND there should also allow
ZERO_EXTEND, and the test for a nested CONST probably has to be replaced
with a test GET_RTX_CLASS (GET_CODE (...)) != RTX_OBJ .

>  
>
>I should be more explicit about it.  In that case, it looks that
>the new optimization reduces
>
>  (zero_extend:SI (truncate:HI (const:SI (sign_extend:SI (truncate:HI (const ...))))))
>
>to
>
>  (zero_extend:SI (truncate:HI (const ...)))
>
>where the inner const is generated by gen_symGOTPLT2reg with VOIDmode
>and this VOIDmode const is caught by the check in simplify_subreg.
>It'd be a logically separated issue as you say but the extend/truncate
>elimination reveals this latent problem.  Should it be submited as an
>independent patch?
>  
>
I am happy with reviewing this either jointly or separately.
However,  I did a grep for sym2GOTPLT, and found that it is used in exactly
one place - the one you want to patch.  If we strip away all the layers 
where
sym2GOTPLT fails to generate the correct mode, all that
is left is a match_operand.  I.e. that pattern serves no purpose.
I think we can replace its sole use with something like:
gen_rtx_CONST (Pmode,
                              gen_rtx_UNSPEC (Pmode, gen_rtvec (1, 
operands[1]),
                                                              
UNSPEC_GOTPLT));