Re: [PATCH-1, rs6000] Add a new type of CC mode - CCBCD for bcd insns [PR100736, PR114732]

["Kewen.Lin" <linkw@linux.ibm.com>]

Hi Haochen,

on 2024/5/30 11:14, HAO CHEN GUI wrote:
> Hi Kewen,
> 
> 在 2024/5/29 13:26, Kewen.Lin 写道:
>> I can understand re-using "unordered" and "eq" will save some efforts than
>> doing with unspecs, but they are actually RTL codes instead of bits on the
>> specific hardware CR, a downside is that people who isn't aware of this
>> design point can have some misunderstanding when reading/checking the code
>> or dumping, from this perspective unspecs (with reasonable name) can be
>> more meaningful.  Normally adopting RTL code is better since they have the
>> chance to be considered (optimized) in generic pass/code, but it isn't the
>> case here as we just use the code itself but not be with the same semantic
>> (meaning).  Looking forward to others' opinions on this, if we want to adopt
>> "unordered" and "eq" like what this patch does, I think we should at least
>> emphasize such points in rs6000-modes.def.
> 
> Thanks so much for your comments. IMHO, the core is if we can re-define
> "unordered" or "eq" for certain CC mode on a specific target. If we can't or
> it's unsafe, we have to use the unspecs. In this case, I just want to define
> the code "unordered" on CCBCD as testing if the bit 3 is set on this CR field.

But my point is that "unordered" has its semantic, it looks a bit tricky to
adopt it on the result from BCD comparison when which only has "invalid" and
"overflow" other than normal ones, though I can understand that this patch
wants to use it to test bit 3 since for float comparison bit 3 is for
"unordered".  However, IMHO it would be more clear to have one unspec to test
bit 3 when bit 3 doesn't actually mean unordered result.

> Actually rs6000 already use "lt" code to test if bit 0 is set for vector
> compare instructions. The following expand is an example.

Yeah, but it doesn't mean it's the most sensible way to do this, IMHO it
suffers from the similar issue and can be improved as well.

> 
> (define_expand "vector_ae_<mode>_p"
>   [(parallel
>     [(set (reg:CC CR6_REGNO)
>           (unspec:CC [(ne:CC (match_operand:VI 1 "vlogical_operand")
>                              (match_operand:VI 2 "vlogical_operand"))]
>            UNSPEC_PREDICATE))
>      (set (match_dup 3)
>           (ne:VI (match_dup 1)
>                  (match_dup 2)))])
>    (set (match_operand:SI 0 "register_operand" "=r")
>         (lt:SI (reg:CC CR6_REGNO)
>                (const_int 0)))
>    (set (match_dup 0)
>         (xor:SI (match_dup 0)
>                 (const_int 1)))]
> 
> I think the "lt" on CC just doesn't mean it compares if CC value is less than an
> integer. It just tests the "lt" bit (bit 0) is set or not on this CC.

But bit 0 doesn't mean for "lt" comparison result in this context any more.

BR,
Kewen

> 
>   Looking forward to your and Segher's further invaluable comments.
> 
> Thanks
> Gui Haochen