Re: [PATCH, rs6000] Fix PR83926, ICE using __builtin_vsx_{div,udiv,mul}_2di builtins

[David Edelsohn <dje.gcc@gmail.com>]

On Mon, Feb 5, 2018 at 9:43 PM, Peter Bergner <bergner@vnet.ibm.com> wrote:
> On 2/5/18 7:32 PM, David Edelsohn wrote:
>> Peter,
>>
>> Why can't you place the tests into the final condition of the pattern
>> so that the pattern fails and the normal GCC fallback machinery is
>> used instead of manually implementing the fallback emulation?
>
> You mean something like the following which I already tried?
>
> (define_expand "div<mode>3"
>   [(set (match_operand:GPR 0 "gpc_reg_operand" "")
>         (div:GPR (match_operand:GPR 1 "gpc_reg_operand" "")
>                  (match_operand:GPR 2 "reg_or_cint_operand" "")))]
>   "<MODE>mode != DImode || TARGET_POWERPC64"
> {
>   if (CONST_INT_P (operands[2])
>       && INTVAL (operands[2]) > 0
>       && exact_log2 (INTVAL (operands[2])) >= 0)
>     {
>       emit_insn (gen_div<mode>3_sra (operands[0], operands[1], operands[2]));
>       DONE;
>     }
>   operands[2] = force_reg (<MODE>mode, operands[2]);
> })
>
>
> The problem with the above is that the condition doesn't seem to be able
> to stop explicit calls to the gen_divdi3() function as long as there is
> some conditions in which the conditional test is true.  Since the condition
> is true for -m64, we create the gen_divdi3() function in insn-emit.c and
> then it seems any explicit calls to that function are fair game, even
> when they come from -m32 compiles.  In this case, the explicit call is
> coming from:
>
> ; Emulate vector with scalar for vec_div in V2DImode
> (define_insn_and_split "vsx_div_v2di"
>   [(set (match_operand:V2DI 0 "vsx_register_operand" "=wa")
>         (unspec:V2DI [(match_operand:V2DI 1 "vsx_register_operand" "wa")
>                       (match_operand:V2DI 2 "vsx_register_operand" "wa")]
>                      UNSPEC_VSX_DIVSD))]
>   "VECTOR_MEM_VSX_P (V2DImode)"
>   "#"
>   "VECTOR_MEM_VSX_P (V2DImode) && !reload_completed"
>   [(const_int 0)]
>   "
> {
>   rtx op0 = operands[0];
>   rtx op1 = operands[1];
>   rtx op2 = operands[2];
>   rtx op3 = gen_reg_rtx (DImode);
>   rtx op4 = gen_reg_rtx (DImode);
>   rtx op5 = gen_reg_rtx (DImode);
>   emit_insn (gen_vsx_extract_v2di (op3, op1, GEN_INT (0)));
>   emit_insn (gen_vsx_extract_v2di (op4, op2, GEN_INT (0)));
>   emit_insn (gen_divdi3 (op5, op3, op4));                       <-- Here
>   emit_insn (gen_vsx_extract_v2di (op3, op1, GEN_INT (1)));
>   emit_insn (gen_vsx_extract_v2di (op4, op2, GEN_INT (1)));
>   emit_insn (gen_divdi3 (op3, op3, op4));                       <-- Here
>   emit_insn (gen_vsx_concat_v2di (op0, op5, op3));
>   DONE;
> }"
>   [(set_attr "type" "div")])
>
> I did also try calling expand_divmod() here which did generate correct
> code, the problem was that it wasn't as clean/optimized as the change
> to gen_divdi3.

Why not fix it at the site of the call to gen_divdi3 instead of the
divdi3 pattern?

Your patch is trying to clean up invocations of methods under
circumstances that they should not be called. This should be avoided
at the call site.  vsx_div_v2di is violating the API when it makes the
call.  vsx_div_v2di knows whether TARGET_POWERPC64 is in effect.
vsx_div_v2di can choose the emulation path.

Your patch seems to be fixing the symptom, not the problem.

Thanks, David