> -----Original Message-----
> From: Tamar Christina <[email protected]>
> Sent: 30 March 2026 13:03
> 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: 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.
Hi Roger,
So how about this,
diff --git a/gcc/config/aarch64/aarch64.cc b/gcc/config/aarch64/aarch64.cc
index 1902a4ee9fa..754a5b76d1d 100644
--- a/gcc/config/aarch64/aarch64.cc
+++ b/gcc/config/aarch64/aarch64.cc
@@ -16234,8 +16234,24 @@ cost_plus:
case GEU:
case LE:
case LEU:
-
- return false; /* All arguments must be in registers. */
+ {
+ op0 = XEXP (x, 0);
+ op1 = XEXP (x, 1);
+ machine_mode inner_mode = GET_MODE (op0);
+ *cost += rtx_cost (op0, inner_mode, code, 0, speed);
+ if (op1 != CONST0_RTX (inner_mode))
+ {
+ unsigned int vec_flags = aarch64_classify_vector_mode (mode);
+ bool unsigned_p = code == LTU || code == LEU || code == GTU
+ || code == GEU;
+ if ((vec_flags & VEC_SVE_DATA) == 0
+ || !aarch64_sve_cmp_immediate_p (op1, !unsigned_p))
+ *cost += rtx_cost (op1, inner_mode, code, 1, speed);
+ if (code == NE && (vec_flags & VEC_ADVSIMD))
+ *cost += COSTS_N_INSNS (1);
+ }
+ return true;
+ }
case FMA:
op0 = XEXP (x, 0);
Which should correctly cost all comparisons. This also fixes the typo in your
original patch:
+ if (op1 != CONST0_RTX (inner_mode))
+ *cost += rtx_cost (op0, inner_mode, NE, 1, speed)
+ + COSTS_N_INSNS (1);
where you meant to cost op1 here.
Thanks,
Tamar
>
> 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
> > > > --
> >
