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
