Re: x86 float conditional move is buggy

[hjl at lucon dot org (H.J. Lu)]

>   > (set (reg/v:DF 26)
>   >         (if_then_else:DF (ne:SI (reg/v:SI 21)
>   >                 (reg/v:SI 22))
>   >             (reg/v:DF 25)
>   >             (reg:DF 48)))
> I don't see anything necessarily wrong with this instruction.  Even if the
> destination must match one of the sources.  Normally such restrictions are
> enforced by the register allocator and reloading pass.

Well, here is the tricky part. On x86, there is one major difference
between normal fp move and conditional fp move, that is for a normal
fp move, the destination can be either already on stack or not on the
stack. reg-stack.c and x86 instructions can deal both cases. But that
is not true for a conditional fp move. In that case, the destination
has to be on the stack to begin with. Otherwise, I don't see there is
a way for reg-stack.c and x86 instructions to put the destination on
the stack.

> 
> I see that the floating point conditional move patterns do not enforce
> any matching operands:
> 
> (define_insn ""
>   [(set (match_operand:SF 0 "register_operand" "=f,f,f,f,f,f")
>         (if_then_else:SF (match_operator 1 "comparison_operator"
>               [(match_operand:QI 2 "nonimmediate_operand" "q,m,q,m,q,m")
>                 (match_operand:QI 3 "general_operand" "qmn,qn,qmn,qn,qmn,qn")])
>               (match_operand:SF 4 "register_operand" "f,f,0,0,f,f")
>               (match_operand:SF 5 "register_operand" "0,0,f,f,f,f")))]
>   "TARGET_CMOVE
>     && GET_CODE (operands[1]) != LT && GET_CODE (operands[1]) != LE
>     && GET_CODE (operands[1]) != GE && GET_CODE (operands[1]) != GT"
>   "#")
> 
> Note the last two alternatives.  They do not enforce any matching.  Nor does
> the insn's condition.
> 

True.

> 
>   > Since on x86, all operands of a fp conditional move pattern have to be
>   > on stack and one of the source operands have to be target operand, this
>   > pattern will lead to the problem. When we see this pattern, we should
>   > either not use conditional move or expand it to
>   > 
>   > (set (reg/v:DF 26) (reg/v:DF 25))
>   > (set (reg/v:DF 26)
>   >         (if_then_else:DF (ne:SI (reg/v:SI 21)
>   >                 (reg/v:SI 22))
>   >             (reg/v:DF 26)
>   >             (reg:DF 48)))
>   > 
>   > Will it work? Can someone provide a patch?
> I think we need to make two changes.
> 
>   1. Generate the code above if neither of the input operands match the
>      output operand.  ie, we're always better off generating good code first.
> 
>   2. Enforce matching in appropriate define_insns.  Either by having the
>      constraints specify that one of the input operands must match the
>      output, or by having the insn's condition enforce the match.
> 
> I'm not sure which of the options in #2 is best for the x86.  Probably using
> the insn's condition as that'll tend to avoid reloads.  However, that could
> also inhibit a series of combinations where the intermediate steps do not
> have a matched operand, but the final step would.
> 

If you take a look at my patch, it does basically what you suggested
here. To do #2, I just took out the last two alternatives.


-- 
H.J. Lu (hjl@gnu.org)