On 9/24/26 2:26 AM, Naveen wrote:
The generic conditional-move expansion currently expands both operands
before looking for a target-specific conditional negate or complement.
This misses idioms such as:
X ? -X : X
X ? ~X : X
and can allow recursive conditional-move expansion when the target-specific
expansion fails.
Recognize conditional negate and complement expressions including reversed
operands and SSA no-op conversions before the existing conditional-move
expansion. Keep the recursion guard active while trying the target
expansion and while falling back to the normal operand expansion.
Add the RISC-V neg<mode>cc pattern for TARGET_ZICOND_LIKE targets. The
pattern expands conditional negation as:
CMP = X ? 1 : 0
MASK = -CMP
DEST = X ^ MASK
DEST += CMP
Avoid this expansion when the operand is a materialized constant since
the existing constant sequence is shorter.
Regression tested on riscv64-unknown-elf. No new failures.
gcc/ChangeLog:
PR target/126245
* expr.cc (strip_ssa_nop_conversions): New.
(get_unary_op_operand): New.
(expand_cond_expr_using_cmove): Recognize conditional negate and
complement expressions and use target-specific expansion while
* riscv.md (neg<mode>cc): New.
gcc/testsuite/ChangeLog:
PR target/126245
* gcc.target/riscv/pr126245.c: New test.
co-authored-by: Andrew Pinski <[email protected]>
Signed-off-by: Naveen <[email protected]>
This seems more complex than I anticipated. So to be clear, not
rejecting, just exploring why that is.
---
gcc/config/riscv/riscv.md | 32 +++++
gcc/expr.cc | 146 ++++++++++++++++++++++
gcc/testsuite/gcc.target/riscv/pr126245.c | 24 ++++
3 files changed, 202 insertions(+)
create mode 100644 gcc/testsuite/gcc.target/riscv/pr126245.c
diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
index 8420b3d29e7..ae57b77cbd0 100644
--- a/gcc/config/riscv/riscv.md
+++ b/gcc/config/riscv/riscv.md
@@ -3504,6 +3504,38 @@
FAIL;
})
+(define_expand "neg<mode>cc"
+ [(set (match_operand:GPR 0 "register_operand")
+ (if_then_else:GPR (match_operand 1 "comparison_operator")
+ (neg:GPR (match_operand:GPR 2 "register_operand"))
+ (match_operand:GPR 3 "register_operand")))]
+ "TARGET_ZICOND_LIKE"
+{
+ rtx cmp = operands[1];
+ rtx cmp0 = XEXP (cmp, 0);
+ rtx cmp1 = XEXP (cmp, 1);
+
+ if (!INTEGRAL_MODE_P (GET_MODE (cmp0))
+ || !rtx_equal_p (operands[2], operands[3]))
+ FAIL;
+
+ rtx_insn *last = get_last_insn_anywhere ();
+ rtx set = last ? single_set (last) : NULL_RTX;
+ if (set
+ && rtx_equal_p (SET_DEST (set), operands[2])
+ && CONST_INT_P (SET_SRC (set)))
+ FAIL;
+
+ rtx cmp_result = gen_reg_rtx (<MODE>mode);
+ riscv_expand_int_scc (cmp_result, GET_CODE (cmp), cmp0, cmp1);
+
+ rtx mask = gen_reg_rtx (<MODE>mode);
+ riscv_emit_unary (NEG, mask, cmp_result);
+ riscv_emit_binary (XOR, operands[0], operands[2], mask);
+ riscv_emit_binary (PLUS, operands[0], operands[0], cmp_result);
+ DONE;
+})
I would probably suggest this as an independent patch as this is useful
on its own without the expr.cc changes (it'll get used via pre-existing
paths in ifcvt.cc).
Drop the TARGET_ZICOND_LIKE test. This is independent of the zicond
style extensions.
Not sure why you're trying to find a prior insn then verify that it's
the same as operands[2] or why you want to verify the source of that set
is a constant integer. It seems this sequence works irrespective of
these conditions.
I would suggest generating another pseudo for the destination of the XOR
so that we're not writing operands[0] twice. There's nothing
functionally wrong with doing that, but there's minor downstream
optimizations that work better if pseudos are only set once.
+/* Strip no-op conversions represented by SSA assignments from EXP. */
What's the motivation here for trying to handle conversions? If we're
really going to try and strip them, we have the STRIP_NOPs family of
macros for that.
@@ -9701,6 +9736,117 @@ expand_cond_expr_using_cmove (tree treeop0
ATTRIBUTE_UNUSED,
expanding_cond_expr_using_cmove = true;
start_sequence ();
+
+ /* Try to expand a conditional negate or complement directly. Expanding
+ the negation first would lose this opportunity and leave the target with
+ a conditional move between X and -X. */
Right. Though selecting between a conditional move vs this custom
sequence may be something we need to cost. A target with true
conditional moves would generate neg+cmov. This sequence is going to be
neg+xor+add. So I think we need to generate both forms and cost the
generated sequence if we tackle this in expr.cc.
+ tree conditional_neg_operand = NULL_TREE;
+ rtx_code conditional_neg_code = UNKNOWN;
+ bool reverse_conditional_neg = false;
+
+ tree other_tree = strip_ssa_nop_conversions (treeop2);
+ tree neg_operand = get_unary_op_operand (treeop1, NEGATE_EXPR);
+ if (neg_operand)
+ conditional_neg_code = NEG;
+ else
+ {
+ neg_operand = get_unary_op_operand (treeop1, BIT_NOT_EXPR);
+ if (neg_operand)
+ conditional_neg_code = NOT;
+ }
+
+ if (neg_operand
+ && operand_equal_p (strip_ssa_nop_conversions (neg_operand),
+ other_tree, 0))
+ conditional_neg_operand = treeop2;
+ else
+ {
+ tree other_neg_operand
+ = get_unary_op_operand (treeop2, NEGATE_EXPR);
+ if (other_neg_operand)
+ conditional_neg_code = NEG;
+ else
+ {
+ other_neg_operand = get_unary_op_operand (treeop2, BIT_NOT_EXPR);
+ if (other_neg_operand)
+ conditional_neg_code = NOT;
+ }
+
+ if (other_neg_operand
+ && operand_equal_p (strip_ssa_nop_conversions (other_neg_operand),
+ strip_ssa_nop_conversions (treeop1), 0))
+ {
+ conditional_neg_operand = treeop1;
+ reverse_conditional_neg = true;
+ }
+ }
So would come kind of canonicalization simplify the hunk above? You're
basically looking for the NEG/NOT in both arms which is great, but it
seems like we ought to be able to make this somewhat cleaner.
+
+ if (conditional_neg_operand != NULL_TREE
+ && !TREE_SIDE_EFFECTS (conditional_neg_operand))
+ {
+ rtx_insn *last = get_last_insn ();
+
+ rtx value = expand_normal (conditional_neg_operand);
+
+ if (TREE_CODE (treeop0) == SSA_NAME
+ && (srcstmt = get_def_for_expr_class (treeop0, tcc_comparison))
+ && !VECTOR_TYPE_P (TREE_TYPE (gimple_assign_rhs1 (srcstmt))))
+ {
+ type = TREE_TYPE (gimple_assign_rhs1 (srcstmt));
+ enum tree_code cmpcode = gimple_assign_rhs_code (srcstmt);
+ op00 = expand_normal (gimple_assign_rhs1 (srcstmt));
+ op01 = expand_normal (gimple_assign_rhs2 (srcstmt));
+ comparison_mode = TYPE_MODE (type);
+ unsignedp = TYPE_UNSIGNED (type);
+ comparison_code = convert_tree_comp_to_rtx (cmpcode, unsignedp);
+ }
+ else if (COMPARISON_CLASS_P (treeop0)
+ && !VECTOR_TYPE_P (TREE_TYPE (TREE_OPERAND (treeop0, 0))))
+ {
+ type = TREE_TYPE (TREE_OPERAND (treeop0, 0));
+ enum tree_code cmpcode = TREE_CODE (treeop0);
+ op00 = expand_normal (TREE_OPERAND (treeop0, 0));
+ op01 = expand_normal (TREE_OPERAND (treeop0, 1));
+ unsignedp = TYPE_UNSIGNED (type);
+ comparison_mode = TYPE_MODE (type);
+ comparison_code = convert_tree_comp_to_rtx (cmpcode, unsignedp);
And this feels similar. The code is almost exactly the same in this two
paths above.
So concretely, let's tackle the RISC-V target bits separately since that
pattern can go in independently of the expr.cc changes.
For the expr.cc changes the big issue is needing to cost both styles of
sequence and pick whichever is lower, try to use STRIP_NOPs rather than
rolling our own and avoid duplicate blocks of code if possible with some
refactoring/canonicalization.
Thanks for tackling this!
jeff
