Re: [PATCH, ARM] correctly encode the CC reg data flow

[Bernd Edlinger <bernd dot edlinger at hotmail dot de>]

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.