Re: [RFC PATCH v1 08/10] ifcvt: add if-conversion to conditional-zero instructions

[Jeff Law <jeffreyalaw@gmail.com>]

On 2/10/23 15:41, Philipp Tomsich wrote:
> Some architectures, as it the case on RISC-V with the proposed
> ZiCondOps and the vendor-defined XVentanaCondOps, define a
> conditional-zero instruction that is equivalent to:
>   - the positive form:  rd = (rc != 0) ? rs : 0
>   - the negated form:   rd = (rc == 0) ? rs : 0
> 
> While noce_try_store_flag_mask will somewhat work for this case, it
> will generate a number of atomic RTX that will misdirect the cost
> calculation and may be too long (i.e., 4 RTX and more) to successfully
> merge at combine-time.
> 
> Instead, we add two new transforms that attempt to build up what we
> define as the canonical form of a conditional-zero expression:
> 
>    (set (match_operand 0 "register_operand" "=r")
>         (and (neg (eq_or_ne (match_operand 1 "register_operand" "r")
>                             (const_int 0)))
>              (match_operand 2 "register_operand" "r")))
> 
> Architectures that provide a conditional-zero are thus expected to
> define an instruction matching this pattern in their backend.
> 
> Based on this, we support the following cases:
>   - noce_try_condzero:
>        a ? a : b
>        a ? b : 0  (and then/else swapped)
>       !a ? b : 0  (and then/else swapped)
>   - noce_try_condzero_arith:
>       conditional-plus, conditional-minus, conditional-and,
>       conditional-or, conditional-xor, conditional-shift,
>       conditional-and
> 
> Given that this is hooked into the CE passes, it is less powerful than
> a tree-pass (e.g., it can not transform cases where an extension, such
> as for uint16_t operations is in either the then or else-branch
> together with the arithmetic) but already covers a good array of cases
> and triggers across SPEC CPU 2017.
> 
> Adding transformations in a tree pass should come in a future
> improvement.
> 
> gcc/ChangeLog:
> 
> 	* ifcvt.cc (noce_emit_insn): Add prototype.
> 	(noce_emit_condzero): Helper for noce_try_condzero and
> 	noce_try_condzero_arith transforms.
> 	(noce_try_condzero): New transform.
> 	(noce_try_condzero_arith): New transform for conditional
> 	arithmetic that can be built up by exploiting that the
> 	conditional-zero instruction will inject 0, which acts
> 	as the neutral element for operations.
> 	(noce_process_if_block): Call noce_try_condzero and
> 	noce_try_condzero_arith.
A few things we've noted while working with this patch internally.

1. The canonical conditional-zero-or-value requires operand 2 to be a 
register.  That will tend to inhibit if-conversion of something like a 
shift-by-constant.  This showed up somewhere in leela.

2. Internally we fixed #1 by forcing that argument into a register when 
it was a constant and reload hasn't completed.  This (naturally) 
resulted in more if-conversions happening.  That in turn caused 
perlbench to fail.  This was tracked down to a conditional-and sequence. 
  I think the call to noce_emit_condzero needs adjustment to pass arg0 
instead of arg1 when OP == AND.

3. The canaonical conditional-zero-or-value assumes the target can do a 
generic SEQ/SNE of two register values.  As you know, on RISC-V we have 
SEQZ/SNEZ.  So we've added another fallback path to handle that case in 
noce_emit_condzero.  You subtract the two values, then you can do an 
SEQZ/SNEZ on the result.




Formatting nits noted below.




> +	  if (!noce_emit_insn (gen_rtx_SET (tmp, cond))) {
> +	    end_sequence ();
> +	    return NULL_RTX;
> +	  }
Open-curly on new line, indented two places from the IF, similarly for 
the close curly.  That will probably require adjusting the indentation 
of the statements.


> +  if (code == NE && cond_arg1 == const0_rtx &&
> +      REG_P (b) && rtx_equal_p (b, cond_arg0))
Don't end with &&, instead bring it down indented one position from the 
open parenthesis.


> +    {
> +      orig_b = b;
> +      b = const0_rtx;
> +    }
> +
> +  /* We may encounter the form "(b == 0) ? b : a", which can be
> +     simplied to "(b == 0) ? 0 : a".  */
> +  if (code == EQ && cond_arg1 == const0_rtx &&
> +      REG_P (b) && rtx_equal_p (b, cond_arg0))
Likewise.


> +    {
> +      target = noce_emit_condzero(if_info, reversep ? a : b, reversep);
Missing whitespace before open-paren of function call.


> +
> +  if (op != PLUS && op != MINUS && op != IOR && op != XOR &&
> +      op != ASHIFT && op != ASHIFTRT && op != LSHIFTRT && op != AND)
Bring the "&&" down to the next line here too.


> +
> +  if (!rtx_equal_p (if_info->x, arg0))
> +    return FALSE;
?!?  What's this for?


> +
> +  target = noce_emit_condzero(if_info, arg1, op != AND ? true : false);
Missing space before open-paren.


> +
> +      emit_insn_before_setloc(seq, if_info->jump,
> +			      INSN_LOCATION(if_info->insn_a));
Here too.

I think the ChangeLog entry for the testsuite needs its filenames 
updated :-)

While this was submitted before stage1 closed, I think we're probably 
too late for it to go forward until we re-open stage1.

jeff