Hi Tamar, Forgive the terminology, I'm not fluent in aarch64, but cpu has a test instruction, so that comparisons against zero are cheaper than comparisons against other values/expressions. If the backend's rtx_costs returned false, the function would recurse and include the overhead of setting a register to zero. The goal is that "(eq x (const_int 0))" is cheaper than "(eq x (const_int 1))".
I hope this explains why the patch is structured the way it is. [As shown by PR 124605, the implementation of rtx_costs in the arm backend is less than ideal]. Cheers, Roger -- > -----Original Message----- > From: Tamar Christina <[email protected]> > Sent: 30 March 2026 12:10 > To: Roger Sayle <[email protected]>; 'GCC Patches' <gcc- > [email protected]> > Cc: 'Richard Biener' <[email protected]>; 'Andrew Pinski' > <[email protected]>; 'Hongtao Liu' <[email protected]> > Subject: RE: [PATCH v2] PR target/123238: VCOND_MASK regression on aarch64 > and x86_64. > > > -----Original Message----- > > From: Roger Sayle <[email protected]> > > Sent: 01 March 2026 12:02 > > To: 'GCC Patches' <[email protected]> > > Cc: 'Richard Biener' <[email protected]>; 'Andrew Pinski' > > <[email protected]>; 'Hongtao Liu' <[email protected]> > > Subject: [PATCH v2] PR target/123238: VCOND_MASK regression on aarch64 > > and x86_64. > > > > > > This is my revised patch for PR target/123238, previously posted at > > https://gcc.gnu.org/pipermail/gcc-patches/2026-February/708283.html > > incorporating the feedback from both Andrew Pinksi and Richard Biener, > > and additionally resolving this code quality regression on aarch64, > > which exhibits many of the same issues as x86_64. Alas, the only > > native aarch64 that I have convenient access to is an Apple M1 laptop, > > so the examples and patches below are against Iain Sandoe's tree for > > aarch64-apple-darwin24.3.0 [as that target isn't yet supported by > > mainline GCC 16]. > > > > To explain and motivate things for the aarch64 maintainers: > > > > void foo(char c[]) > > { > > for (int i = 0; i < 8; i++) > > c[i] = c[i] != 'a' ? 'c' : 'e'; } > > > > currently generates with -O2 the following: > > > > foo: movi v30.8b, 0x61 > > ldr d0, [x0] > > movi v29.8b, 0x63 > > movi v31.8b, 0x65 > > cmeq v30.8b, v0.8b, v30.8b > > not v30.8b, v30.8b > > bit v31.8b, v29.8b, v30.8b > > str d31, [x0] > > ret > > > > where a cmeq followed by a not is used to implement NE_EXPR. > > c.f. the comment "Handle NE as !EQ" in aarch64-simd.md's expander of > > vec_cmp<mode><mode>. With this patch, including the change to > > aarch64_rtx_costs to indicate that NE is more expensive than EQ, the > > middle-end swaps the VCOND_EXPR, reducing the number of instructions > > in the example above [to what it was in GCC 14]. > > > > Changes from the previous version of this patch include Richard's > > recommendation to use get_gimple_for_ssa_name instead of > > SSA_NAME_DEF_STMT, and Andrew's suggestion to log expand's decision to > > the dump file, which now contains lines such as: > > > > ;; swapping operands of .VCOND_MASK > > ;; cost of original ne: 8 > > ;; cost of replacement eq: 4 > > > > or (for failure) > > > > ;; not swapping operands of .VCOND_MASK ;; cost of original eq: 4 ;; > > cost of replacement ne: 8 > > > > I've kept the handling of an embedded tcc_comparison, this both helps > > document what the code intends to do, and (defensively) supports > > expand's ability to work with GENERIC, and not just GIMPLE trees (with > > or without TER). During RTL expansion, the middle-end may temporarily > > construct trees (for intermediate > > expressions) that aren't always valid in earlier passes. > > > > Finally, I've no objection to someone else attempting to perform this > > type of (target dependent) optimization during the gimple-isel pass, > > though I expect it will be difficult to maintain hardware specific > > idioms, such as aarch64's representation of NE as !EQ (in some machine > > modes) when match.pd and elsewhere in the middle-end attempts to keep > > tree's canonical (and machine independent) by reducing operators, > > commuting constants in binary expressions etc. > > > > I've previously explained how the ISEL pass adversely affects code > > generation on x86_64 (making decisions about lowering that are better > > left to the backends), but the same issue is also there for aarch64. > > > > Consider the (closely related) case: > > > > void bar(char c[]) > > { > > for (int i = 0; i < 8; i++) > > c[i] = c[i] != 'a'; > > } > > > > Here gimple-isel makes the decision to turn a 0/-1 vector to a > > 0/1 vector using bitwise-AND with 1, so on aarch64 we generate: > > > > bar: movi v30.8b, 0x61 > > ldr d0, [x0] > > movi v31.8b, 0x1 > > cmeq v30.8b, v0.8b, v30.8b > > bic v31.8b, v31.8b, v30.8b > > str d31, [x0] > > ret > > > > Ideally, aarch64 should be able to use "ushr v31.8b, v31.8b, 7" > > but once the .VCOND_MASK is lowered to an AND, the semantics that the > > original vector was a -1/0 mask has been lost, so later passes are > > unable to improve things. Alas because gimple-isel thinks it knows > > best, GCC even de-optimizes code, introducing instructions into: > > > > typedef char v8qi __attribute__((vector_size(8))); void baz(v8qi *c) { > > v8qi t = *c; > > t = t == (v8qi){'a','a','a','a','a','a','a','a'}; > > t >>= 7; > > *c = t; > > } > > > > In theory, one might be able to work around the above glitch with > > -fdisable-tree-isel but that currently ICEs, and would need my > > (rejected) patch from May 2022: > > https://gcc.gnu.org/pipermail/gcc-patches/2022-May/595382.html > > But this is digressing far off topic. > > > > This patch has been tested on x86_64-pc-linux-gnu with make bootstrap > > and make -k check, both with and without --target_board=unix{-m32} > > with no new failures. It has also been tested on iains' > > aarch64-apple-darwin24.3.0 also with no new failures. > > Ok for mainline? (the middle-end, aarch64 and x86_64 pieces can all > > be approved/applied independently). > > > > Sorry for the very late reply. I was on holidays. > > > diff --git a/gcc/config/aarch64/aarch64.cc > > b/gcc/config/aarch64/aarch64.cc index f2ecb0ee8cb..d92a5aa491c 100644 > > --- a/gcc/config/aarch64/aarch64.cc > > +++ b/gcc/config/aarch64/aarch64.cc > > @@ -16224,8 +16224,34 @@ cost_plus: > > return aarch64_if_then_else_costs (XEXP (x, 0), XEXP (x, 1), > > XEXP (x, 2), cost, speed); > > > > - case EQ: > > case NE: > > + if (GET_MODE_CLASS (mode) == MODE_VECTOR_INT) > > SVE does have CMPNE as an operation, so this would be incorrect for it. > I think you want instead > > if (aarch64_classify_vector_mode (mode) & VEC_ADVSIMD) > > > + { > > + op0 = XEXP (x, 0); > > + op1 = XEXP (x, 1); > > + machine_mode inner_mode = GET_MODE (op0); > > + *cost += rtx_cost (op0, inner_mode, NE, 0, speed); > > + /* NE costs one_cmpl<mode>2 more than EQ. */ > > + if (op1 != CONST0_RTX (inner_mode)) > > + *cost += rtx_cost (op0, inner_mode, NE, 1, speed) > > + + COSTS_N_INSNS (1); > > + return true; > > + } > > Isn't this the same as > > op0 = XEXP (x, 0); > if (op0 != CONST0_RTX (GET_MODE (op0);)) > *cost += COSTS_N_INSNS (1); > return false; > > since the false will cost the operands appropriately. > > > + return false; /* All arguments need to be in registers. */ > > + > > + case EQ: > > + if (GET_MODE_CLASS (mode) == MODE_VECTOR_INT) > > + { > > + op0 = XEXP (x, 0); > > + op1 = XEXP (x, 1); > > + machine_mode inner_mode = GET_MODE (op0); > > + *cost += rtx_cost (op0, inner_mode, EQ, 0, speed); > > + if (op1 != CONST0_RTX (inner_mode)) > > + *cost += rtx_cost (op0, inner_mode, EQ, 1, speed); > > + return true; > > + } > > Do we need this hunk? Returning false should have it iterate over the operands > and cost them already. So I think we can drop it? > > Thanks, > Tamar > > > + return false; /* All arguments must be in registers. */ > > + > > case GT: > > case GTU: > > case LT: > > > Thanks in advance (and for the previous reviews). > > > > > > 2026-03-01 Roger Sayle <[email protected]> > > > > gcc/ChangeLog > > PR target/123238 > > * expr.cc (convert_tree_comp_to_rtx): Make global. > > * expr.h (convert_tree_comp_to_rtx): Prototype here. > > * internal-fn.cc (expand_vec_cond_mask_optab_fn): Use rtx_costs > > to determine whether swapping operands would result in better > > code. > > > > * config/aarch64/aarch64.cc (aarch64_rtx_costs) <case NE/EQ>: > > Provide costs for integer vector equality and inequality. > > > > * config/i386/i386-expand.cc (ix86_expand_int_vec_cmp): On > > AVX512 targets use a ternlog instead of a comparison to negate > > the mask (requires one instruction instead of two). > > * config/i386/i386.cc (ix86_rtx_costs): Refactor code for UNSPEC. > > Provide costs for UNSPEC_BLENDV and UNSPEC_MOVMSK. Provide > > costs for comparison operators of integer vector modes. > > > > gcc/testsuite/ChangeLog > > PR target/123238 > > * gcc.target/aarch64/pr123238.c: New test case. > > * gcc.target/i386/pr123238.c: Likewise. > > > > Roger > > --
