Re: [PATCH] RISC-V:Add the MIPS P8700 conditional move extension instruction support.

[Jeff Law]

On 6/5/25 2:30 AM, Umesh Kalappa wrote:
Thank you @Jeff Law <mailto:jeffreya...@gmail.com>  for the initial 
comments ,yes will update the ChangeLog accordingly and typo fix and
 >>My biggest concern is overall structure of 
riscv_expand_conditional_move.  I kind of get the sense
 >>that we need to refactor operand canonicalization into its own routine,
 >>then have two subroutines,
Well ,it makes sense to refactor to have two subroutines .

Thank you again  for your  time ,we will make changes and will send the 
updated patch soon .
Note I was just looking a this code today for unrelated reasons.  I 
don't *think* refactoring will be that tough.  But I did spot an 
interaction  between the condition canonicalization and the rest of the 
code.

In particular if we don't have special insns like zicond, theadcmove, 
etc, we have fallback code which looks like this:
     else if (!TARGET_ZICOND_LIKE)
        {
          if (invert)
            std::swap (cons, alt);

          rtx reg1 = gen_reg_rtx (mode);
          rtx reg2 = gen_reg_rtx (mode);
          rtx reg3 = gen_reg_rtx (mode);
          rtx reg4 = gen_reg_rtx (mode);

          riscv_emit_unary (NEG, reg1, tmp);
          riscv_emit_binary (AND, reg2, reg1, cons);
          riscv_emit_unary (NOT, reg3, reg1);
          riscv_emit_binary (AND, reg4, reg3, alt);
          riscv_emit_binary (IOR, dest, reg2, reg4);
          return true;
        }


That code is dependent on this blob of code in the condition 
canonicalization:

      /* In the fallback generic case use MODE rather than WORD_MODE for
         the output of the SCC instruction, to match the mode of the NEG
         operation below.  The output of SCC is 0 or 1 boolean, so it is
         valid for input in any scalar integer mode.  */
      rtx tmp = gen_reg_rtx ((TARGET_ZICOND_LIKE
                              || TARGET_SFB_ALU || TARGET_XTHEADCONDMOV)
                             ? word_mode : mode);


So we'll need to untangle that a bit.  But it also reinforces my gut 
feeling that we need to do a bit of refactoring.  I'd forgotten that we 
had the fallback path as well as the SFB_ALU stuff and other oddities.

I am playing with a change which separates the mode of the conditional's 
arguments from the mode of the output and true/false arms (which is how 
I spotted the issue above).   That will help a tiny bit with the 
refactoring I hope.

Jeff