Bug 125731 - Improve RISC-V sequence for conditional xor with a constant
Summary: Improve RISC-V sequence for conditional xor with a constant
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: rtl-optimization (show other bugs)
Version: 17.0
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords: easyhack, missed-optimization
Depends on:
Blocks: 124956
  Show dependency treegraph
 
Reported: 2026-06-10 22:53 UTC by Jeffrey A. Law
Modified: 2026-08-14 22:44 UTC (History)
2 users (show)

See Also:
Host:
Target: riscv64
Build:
Known to work:
Known to fail:
Last reconfirmed: 2026-06-15 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jeffrey A. Law 2026-06-10 22:53:00 UTC
Given this function:


long fun_not1 (int a, long b)
{
    if (!(a & 1))
        b ^= 8;
    return b;
}

Compiled with -march=rv64gcbv_zicond -O2 we currently get:

fun_not1:
        andi    a5,a0,1
        mv      a0,a1
        bne     a5,zero,.L3
        xori    a0,a1,8
.L3:
        ret

But it's really just a conditional xor.  We could instead generate:

        andi    a5,a0,1
        li      a4,8
        czero.nez       a5,a4,a5
        xor     a0,a1,a5
        ret

It's the same number of instructions, but if the conditional branch is not easily predictable, the branchless sequence will be much faster.  There is an alternate sequence that avoids the zicond extension, but it's likely marginally slower and with zicond part of the rva23 profile, I doubt it's worth meaningful time to try to generate the non-zicond sequence.

Like so many simplification issues, this can be handled in both gimple and RTL.  I want this BZ to focus on the RTL simplification.  There's already a BZ that effectively covers the gimple side (pr118360).

I think this is a failure in ifcvt.cc and fixing it would be a step towards fixing pr124956 as well.

In particular ifcvt isn't handling the case where we have a CONST_INT operand.  It's requiring REG/SUBREG expressions only.  See ifcvt.cc::get_base_reg and its callers.  We need to change that routine to allow CONST_INTs (which will require a name change) and likely other minor changes since CONST_INT nodes do not have a mode and I think we use the return value from this routine to generate a new pseudo register (which needs a valid mode).
Comment 1 Drea Pinski 2026-06-15 06:39:25 UTC
.
Comment 2 Jeffrey A. Law 2026-06-19 06:04:18 UTC
Fixing this would likely also help the testcase in pr3507 when compiled on RISC-V.
Comment 3 Jeffrey A. Law 2026-07-13 19:27:32 UTC
Worth noting that fixing this will trigger regressions on loongarch.  Loongarch seems to explcitly want prefer a sequence like scc+sll+add for a conditional add by a power of 2 rather than a lui+maskeq+add.

I'm currently working on a prerequisite improvement to ifcvt that allows us to use the shift based sequences to select across A and A OP C where C is 2^n.
Comment 4 GCC Commits 2026-07-19 14:13:04 UTC
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:a33f26607eb4f3e3d68e575a0395401c940cf1c7

commit r17-2519-ga33f26607eb4f3e3d68e575a0395401c940cf1c7
Author: Jeff Law <jeffrey.law@oss.qualcomm.com>
Date:   Sun Jul 19 08:11:23 2026 -0600

    [committed] Improve select across A/A OP C where C is 2^n
    
    Testing for PR125731 exposed a bit of unexpected behavior on loongarch.
    
    Basically the PR125731 patch allows if-conversion to again handle generating
    conditional zero based sequences where one of the two operands in the original
    sequence was a constant integer.  Generating a conditional zero based sequence
    usually results in better code than a generalized conditional move.
    
    Discovering those cases better led to a regression on loongarch which seems to
    want to generate even more specialized sequences than a conditional select
    across 0,2^n for things like conditional add.  Consider this loongarch
    assembly:
    
            sltu    $r12,$r0,$r12
            slli.d  $r12,$r12,16
            add.d   $r14,$r14,$r12
    
    That's a conditional add by 65536.   We use the sltu to generate 1/0, shift
    that by 16 generating 65536/0, then add that result to the other operand.  With
    the work for PR125731 we get this instead:
    
            lu12i.w $r16,16                 # 0x10
            maskeqz $r12,$r16,$r12
            add.d   $r14,$r14,$r12
    
    Normally I would prefer the 2nd sequence as the high part load has no
    dependencies and can issue whenever is convenient, but loongarch explicitly
    prefers the first sequence and I'm willing to assume that was done for a good
    reason.  For RISC-V it's probably a toss-up.  The first form likely compresses
    better and doesn't rely on zicond, but the second form has one less incoming
    dependency.  Barring hard data, I'm going to declare them equivalent and target
    the sequence loongarch wants.
    
    Thankfully this is a class of problems that's been on my radar for a while.
    
    Given a select across A and A OP C where C is 2^N we can left shift the result
    of the SCC to give us a select across 0 and 2^n, then we emit A OP X (where X
    holds the result of that left shift).  We can do this add, sub, shifts,
    rotates, ior, xor, basically anything where "0" is a neutral operand.  That
    obviously excludes AND where -1 is the neutral.  That's ultimately the same set
    of operators as the condzero arithmetic supports except we'd need to filter out
    AND.
    
    The implementation is structured similar to store_flag_constants, though
    simplified where obviously possible.
    
    If we look at a couple subtests within the loongarch conditional-move-opt-1.c
    testcase, but compiling for RISC-V:
    
    extern long lm, lr;
    
    void
    test_nez ()
    {
      if (lm != 0)
        lr <<= (1 << 4);
      lr += lm;
    }
    
    void
    test_eqz ()
    {
      if (lm == 0)
        lr >>= (1 << 2);
      lr += lm;
    }
    
    The relevant conditional move sequences look like this:
    
            slli    a3,a5,16
            czero.eqz       t1,a3,t0
            czero.nez       t2,a5,t0
            add     a0,t2,t1
            add     a1,t0,a0
    
    and:
    
            srai    a3,a5,4
            czero.nez       t1,a3,t0
            czero.eqz       t2,a5,t0
            add     a0,t2,t1
            add     a1,t0,a0
    
    Not bad, but with this patch we clearly do better:
    
            snez    a3,t0
            slli    t1,a3,4
            sll     t2,a4,t1
            add     a0,t0,t2
    
    and
    
            seqz    a3,t0
            slli    t1,a3,2
            sra     t2,a4,t1
            add     a0,t0,t2
    
    Probably the same performance as the czeros can execute in parallel, but it's
    smaller from an encoding standpoint and doesn't require zicond.
    
    Bootstrapped and regression tested on x86_64, alpha, armv7, loongarch64,
    riscv64 (k3, k1 and c920). Probably others as well, though I didn't check other
    natives explicitly to see if it'd picked up the latest version of the patch.
    Interestingly enough this does trigger meaningfully during bootstraps on
    various targets as I stumbled across multiple failures due to a couple logic
    errors in earlier versions.
    
    There's still things that could be improved in here.  Most obviously AND
    handling, cases where STORE_FLAG_VALUE != 1 (which likely work due to
    normalization, but our ability to test is limited), exploiting negated logicals
    for things like conditional bit clear, etc.  Even with the limitations, this
    seems worthwhile to go forward now.
    
    Pushing to the trunk.
    
            PR target/125731
    
    gcc/
            * ifcvt.cc (noce_cond_zero_binary_op_supported): Move earlier.
            (noce_try_shifted_store_flag): New function.
            (noce_process_if_block): Use it.
    
    gcc/testsuite
            * gcc.target/riscv/pr125731-1.c: New test.
            * gcc.target/riscv/rvv/vsetvl/vsetvl-15.c: Drop shift count test.
Comment 5 Avinal Kumar 2026-08-04 07:58:15 UTC
I am assuming the commit mentioned in the last comment fulfills the prerequisite. If yes, I can work on this.
Comment 6 Jeffrey A. Law 2026-08-14 04:37:07 UTC
Sorry, I'd been out of the office much of the last couple weeks.  Yes, the patch I installed about a month ago is the prequisite patch.    However, Shreya Munnangi has already sent me a patch to address this problem that I just need to review and test.
Comment 7 GCC Commits 2026-08-14 20:55:31 UTC
The master branch has been updated by Jeff Law <law@gcc.gnu.org>:

https://gcc.gnu.org/g:af0cf36fb47a704a02dd2ead5f8db5d140e45351

commit r17-3306-gaf0cf36fb47a704a02dd2ead5f8db5d140e45351
Author: Shreya Munnangi <smunnang@qti.qualcomm.com>
Date:   Fri Aug 14 14:53:34 2026 -0600

    [PATCH][RISC-V][PR rtl-optimization/125731] Improving sequences for conditional xor with a constant
    
    This is Shreya's work to address pr125731.  I did some testing around this to
    verify riscv64-elf and riscv32-elf are happy.  Testing on the K1/K3/c920 will
    fire up tonight, but not expecting significant issues there.
    
    --
    
    In PR125731,
    
    We are generating inefficient sequences for conditional xor with a
    constant. Given this testcase,
    
    long fun_not1 (int a, long b)
    {
        if (!(a & 1))
            b ^= 8;
        return b;
    }
    
    We are generating:
    
    fun_not1:
            andi    a5,a0,1
            mv      a0,a1
            bne     a5,zero,.L3
            xori    a0,a1,8
    .L3:
            ret
    
    However, this can be simplified into a branchless sequence.  One such sequence:
    
           andi    a5,a0,1
           li      a4,8
           czero.nez       a5,a4,a5
           xor     a0,a1,a5
           ret
    
    Another form:
    
            andi    a0,a0,1
            seqz    t0,a0
            slli    t1,t0,3
            xor     a0,a1,t1
            ret
    -- I've edited Shreya's explanation a bit --
    
    The if-converter's get_base_reg currently returns NULL if presented with a
    constant and that inhibits if conversion through the noce_try_cond_arith which
    can generate more efficient sequences than generalized conditional moves.  So
    the first thing we need to do is support constants in get_base_reg.
    
    The return value from get_base_reg is used to generate a new pseudo register,
    so if get_base_reg starts returning constants, then we're going to run into
    problems generating the new pseudo because constants are VOIDmode.
    
    I changed the name to get_base_reg_or_constant to reflect that we are also
    handling constants, not just returning the base registers of a reg or subreg.
    The point where the caller uses the return value to get a mode for the new
    pseudo has been changed to get the mode from the other operand of the binary
    operation.
    
    Those changes are sufficient to if-convert this example as well as enabling
    more efficient code generation for other cases where we have a conditional
    operation where one operand is a constant.
    
    Although the newly generated sequence is the same number of insns, it will be
    much faster if the conditional branch is not easily predictable.
    
            PR rtl-optimization/125731
    gcc/
            * ifcvt.cc (get_base_reg_or_const): Renamed from get_base_reg.
            Handle CONST_INTs too.
            (noce_try_cond_arith): Use get_base_reg_or_const.  Handle case where
            get_base_reg_or_const returns a CONST_INT.
    
    gcc/testsuite
            * gcc.target/riscv/pr125731.c: New test.
Comment 8 Jeffrey A. Law 2026-08-14 20:56:25 UTC
Fixed by Shreya's patch on the trunk.
Comment 9 Jeffrey A. Law 2026-08-14 22:44:47 UTC
.