Re: [PATCH] avoid xmm register in SSE float->int conversions

[Uros Bizjak <uros@kss-loka.si>]

Jan Hubicka wrote:

>>For the DF->SI testcases like this:
>>
>>int test1(double a) {
>>       return sin(a);
>>}
>>
>>current mainline gcc produces following code for -march=pentium4:
>>       ...
>>       fsin
>>       fstpl   (%esp)
>>       movsd   (%esp), %xmm0
>>       cvttsd2si       %xmm0, %eax
>>       ...
>>       ret
>>
>>At least for pentium4 (and perhaps other !TARGET_K8 targets), xmm 
>>register could be avoided, as cvttsd2si can convert directly from memory 
>>to integer register. Attached patch introduces a couple of peephole2 
>>optimizers to get rid of extra movsd.
>>Unfortunatelly, combine pass can't be used in this case, because all 
>>necessary moves are generated in greg pass _after_ combine.
>>    
>>
>
>To make the peepholes safe, you need to verify that xmm0 is dead after
>it is stored to memoery again.  However it is definitly better to teach
>reload to not do this sillyness, but it might be quite dificult...
>
>  
>
Hm, I don't see where xmm0 is stored to memory... Perhaps you thought 
that it is necessary to check if xmm0 is dead after it is converted to 
register? Peephole is transforming sequence:

 >>>
(insn 34 31 35 0 (set (mem:DF (plus:SI (reg/f:SI 6 bp)
                (const_int -8 [0xfffffff8])) [0 S8 A8])
        (reg:DF 8 st [61])) 67 {*movdf_nointeger} (nil)
    (expr_list:REG_DEAD (reg:DF 8 st [61])
        (nil)))

(insn 35 34 13 0 (set (reg:DF 21 xmm0)
        (mem:DF (plus:SI (reg/f:SI 6 bp)
                (const_int -8 [0xfffffff8])) [0 S8 A8])) 67 
{*movdf_nointeger} (nil)
    (nil))

(insn:HI 13 35 17 0 (set (reg:SI 0 ax [60])
        (fix:SI (reg:DF 21 xmm0))) 121 {fix_truncdfsi_sse} 
(insn_list:REG_DEP_TRUE 12 (nil))
    (expr_list:REG_DEAD (reg:DF 21 xmm0)
        (nil)))

 >>>
into
 >>>

(insn 34 31 44 0 (set (mem:DF (plus:SI (reg/f:SI 6 bp)
                (const_int -8 [0xfffffff8])) [0 S8 A8])
        (reg:DF 8 st [61])) 67 {*movdf_nointeger} (nil)
    (expr_list:REG_DEAD (reg:DF 8 st [61])
        (nil)))

(insn 44 34 17 0 (set (reg:SI 0 ax [60])
        (fix:SI (mem:DF (plus:SI (reg/f:SI 6 bp)
                    (const_int -8 [0xfffffff8])) [0 S8 A8]))) -1 (nil)
    (nil))
 >>>

It would be a trivial task to add a check for dead register into 
peephole2 pattern constraint.

However, it looks that this would be only a surface fix for deeper 
problems. fix_truncdfsi_sse pattern is defined as:

(define_insn "fix_truncdfsi_sse"
  [(set (match_operand:SI 0 "register_operand" "=r,r")
    (fix:SI (match_operand:DF 1 "nonimmediate_operand" "Y,Ym")))]

and its (operand 1) constraints show that this insn accepts memory 
inputs.  Changing input constraint from "Y,Ym" to "m,m" shows  the problem:

ccc.c:3: error: unable to generate reloads for:
(insn:HI 13 31 17 0 (set (reg:SI 0 ax [60])
        (fix:SI (reg:DF 8 st [61]))) 115 {fix_truncdfsi_sse} 
(insn_list:REG_DEP_TRUE 12 (nil))
    (expr_list:REG_DEAD (reg:DF 8 st [61])
        (nil)))
ccc.c:3: internal compiler error: in find_reloads, at reload.c:3676

Reload is unable to transfer operands via memory. Ideally, it should 
allocate a local stack slot and move/copy FP register to this slot. This 
would satisfy fix_truncdfsi_sse constranints. Indeed, looking in i386.md 
this comment (near disabled "*ficom_1" pattern) has been found:

;; %%% Play games with accepting gp registers, as otherwise we have to
;; force them to memory during rtl generation, which is no good.  We
;; can get rid of this once we teach reload to do memory input reloads
;; via pushes.

It is unclear to me, if this is a design choice, because memory input 
reloads would be benefical for number of instructions in i386's case. 
Consider for example "*fix_trunc?i_1". Reload coud allocate a slot for 
its output automatically, without using "assign_386_stack_local (DImode, 
0);".

Uros.