> -----Original Message----- > From: Roger Sayle <[email protected]> > Sent: 30 March 2026 12:37 > To: Tamar Christina <[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. > > > 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))".
Oh I see, yeah, but that's the case for all comparisons then, including scalar and unordered since scalar uses the zero register and for vector both SVE and Adv.SIMD support zero immediates, and so does scalar FP. So then the correct costing would be to handle zero for all comparisons and only increase the cost of NE with non-zero for Adv. SIMD. Let me run that change through regtest and benchmarking and will come back to you in a bit. Cheers, Tamar > > 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 > > > -- >
