On 01/13/17 14:50, Richard Earnshaw (lists) wrote:
> On 18/12/16 12:58, Bernd Edlinger wrote:
>> Hi,
>>
>> this is related to PR77308, the follow-up patch will depend on this one.
>>
>> When trying the split the *arm_cmpdi_insn and *arm_cmpdi_unsigned
>> before reload, a mis-compilation in libgcc function __gnu_satfractdasq
>> was discovered, see [1] for more details.
>>
>> The reason seems to be that when the *arm_cmpdi_insn is directly
>> followed by a *arm_cmpdi_unsigned instruction, both are split
>> up into this:
>>
>> [(set (reg:CC CC_REGNUM)
>> (compare:CC (match_dup 0) (match_dup 1)))
>> (parallel [(set (reg:CC CC_REGNUM)
>> (compare:CC (match_dup 3) (match_dup 4)))
>> (set (match_dup 2)
>> (minus:SI (match_dup 5)
>> (ltu:SI (reg:CC_C CC_REGNUM) (const_int
>> 0))))])]
>>
>> [(set (reg:CC CC_REGNUM)
>> (compare:CC (match_dup 2) (match_dup 3)))
>> (cond_exec (eq:SI (reg:CC CC_REGNUM) (const_int 0))
>> (set (reg:CC CC_REGNUM)
>> (compare:CC (match_dup 0) (match_dup 1))))]
>>
>> The problem is that the reg:CC from the *subsi3_carryin_compare
>> is not mentioning that the reg:CC is also dependent on the reg:CC
>> from before. Therefore the *arm_cmpsi_insn appears to be
>> redundant and thus got removed, because the data values are identical.
>>
>> I think that applies to a number of similar pattern where data
>> flow is happening through the CC reg.
>>
>> So this is a kind of correctness issue, and should be fixed
>> independently from the optimization issue PR77308.
>>
>> Therefore I think the patterns need to specify the true
>> value that will be in the CC reg, in order for cse to
>> know what the instructions are really doing.
>>
>>
>> Bootstrapped and reg-tested on arm-linux-gnueabihf.
>> Is it OK for trunk?
>>
>
> I agree you've found a valid problem here, but I have some issues with
> the patch itself.
>
>
> (define_insn_and_split "subdi3_compare1"
> [(set (reg:CC_NCV CC_REGNUM)
> (compare:CC_NCV
> (match_operand:DI 1 "register_operand" "r")
> (match_operand:DI 2 "register_operand" "r")))
> (set (match_operand:DI 0 "register_operand" "=&r")
> (minus:DI (match_dup 1) (match_dup 2)))]
> "TARGET_32BIT"
> "#"
> "&& reload_completed"
> [(parallel [(set (reg:CC CC_REGNUM)
> (compare:CC (match_dup 1) (match_dup 2)))
> (set (match_dup 0) (minus:SI (match_dup 1) (match_dup 2)))])
> (parallel [(set (reg:CC_C CC_REGNUM)
> (compare:CC_C
> (zero_extend:DI (match_dup 4))
> (plus:DI (zero_extend:DI (match_dup 5))
> (ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
> (set (match_dup 3)
> (minus:SI (minus:SI (match_dup 4) (match_dup 5))
> (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0))))])]
>
>
> This pattern is now no-longer self consistent in that before the split
> the overall result for the condition register is in mode CC_NCV, but
> afterwards it is just CC_C.
>
> I think CC_NCV is correct mode (the N, C and V bits all correctly
> reflect the result of the 64-bit comparison), but that then implies that
> the cc mode of subsi3_carryin_compare is incorrect as well and should in
> fact also be CC_NCV. Thinking about this pattern, I'm inclined to agree
> that CC_NCV is the correct mode for this operation
>
> I'm not sure if there are other consequences that will fall out from
> fixing this (it's possible that we might need a change to select_cc_mode
> as well).
>
Yes, this is still a bit awkward...
The N and V bit will be the correct result for the subdi3_compare1
a 64-bit comparison, but zero_extend:DI (match_dup 4) (plus:DI ...)
only gets the C bit correct, the expression for N and V is a different
one.
It probably works, because the subsi3_carryin_compare instruction sets
more CC bits than the pattern does explicitly specify the value.
We know the subsi3_carryin_compare also computes the NV bits, but it is
hard to write down the correct rtl expression for it.
In theory the pattern should describe everything correctly,
maybe, like:
set (reg:CC_C CC_REGNUM)
(compare:CC_C
(zero_extend:DI (match_dup 4))
(plus:DI (zero_extend:DI (match_dup 5))
(ltu:DI (reg:CC_C CC_REGNUM) (const_int 0)))))
set (reg:CC_NV CC_REGNUM)
(compare:CC_NV
(match_dup 4))
(plus:SI (match_dup 5) (ltu:SI (reg:CC_C CC_REGNUM) (const_int 0)))
set (match_dup 3)
(minus:SI (minus:SI (match_dup 4) (match_dup 5))
(ltu:SI (reg:CC_C CC_REGNUM) (const_int 0)))))
But I doubt that will work to set CC_REGNUM with two different modes
in parallel?
Another idea would be to invent a CC_CNV_NOOV mode, that implicitly
defines C from the DImode result, and NV from the SImode result,
similar to the CC_NOOVmode, that also leaves something open what
bits it really defines?
What do you think?
Thanks
Bernd.
