rva22u64_v regression on 525.x264_r

[lukel97]

Between 6e498bc and c7579bf, the SPEC CPU 2017 benchmark 525.x264_r has regressed about ~9% on the spacemit-x60, built with -march=rva22u64_v -O3 -flto: https://lnt.lukelau.me/db_default/v4/nts/graph?highlight_run=133&plot.0=25.13.3

It looks like this is made up of several independent regressions over the last few days since the 23rd of January, the first one being a 6% regression between 6e498bc and 26fc07d.

The cause of the regression is mostly due to the function pixel_hadamard_ac being unprofitably SLP vectorized in a lot more places, which can be viewed in the profile here: https://lnt.lukelau.me/db_default/v4/nts/profile/13/133/130

Noticeably there are now 8 new LMUL 4 vrgatherei16.vvs which are very expensive.

There were a few SLP related changes landed during this period which may be relevant, #123896 #124254 #124129.

Are we missing something from the RISC-V cost-model? cc) @alexey-bataev @RKSimon @HanKuanChen

[llvmbot]

@llvm/issue-subscribers-backend-risc-v

Author: Luke Lau (lukel97)

Between 6e498bc and c7579bf, the SPEC CPU 2017 benchmark 525.x264_r has regressed about ~9% on the spacemit-x60: https://lnt.lukelau.me/db_default/v4/nts/graph?highlight_run=133&plot.0=25.13.3

It looks like this is made up of several independent regressions over the last few days since the 23rd of January, the first one being a 6% regression between 6e498bc and 26fc07d.

The cause of the regression is mostly due to the function pixel_hadamard_ac being unprofitably SLP vectorized in a lot more places, which can be viewed in the profile here: https://lnt.lukelau.me/db_default/v4/nts/profile/13/133/130

Noticeably there are now 8 new LMUL 4 vrgatherei16.vvs which are very expensive.

There were a few SLP related changes landed during this period which may be relevant, #123896 #124254 #124129.

Are we missing something from the RISC-V cost-model? cc) @alexey-bataev @RKSimon @HanKuanChen

[RKSimon]

@lukel97 Are you able to reduce this to a test case or godbolt link please?

[RKSimon]

Are we missing something from the RISC-V cost-model? cc) @alexey-bataev @RKSimon @HanKuanChen

If it is this we need to isolate the intrinsic call that has been altered

[lukel97]

@RKSimon Here's the function extracted from https://github.com/mirror/x264/blob/master/common/pixel.c, clang -target riscv64 -march rva22u64_v -O3 should reproduce the codegen:

#include <stdint.h>


static uint32_t abs2( uint32_t a )
{
    uint32_t s = ((a>>15)&0x10001)*0xffff;
    return (a+s)^s;
}

#define HADAMARD4(d0, d1, d2, d3, s0, s1, s2, s3) {\
    int t0 = s0 + s1;\
    int t1 = s0 - s1;\
    int t2 = s2 + s3;\
    int t3 = s2 - s3;\
    d0 = t0 + t2;\
    d2 = t0 - t2;\
    d1 = t1 + t3;\
    d3 = t1 - t3;\
}

uint64_t pixel_hadamard_ac( uint8_t *pix, int stride )
{
    uint32_t tmp[32];
    uint32_t a0, a1, a2, a3, dc;
    int sum4 = 0, sum8 = 0;
    for( int i = 0; i < 8; i++, pix+=stride )
    {
        uint32_t *t = tmp + (i&3) + (i&4)*4;
        a0 = (pix[0]+pix[1]) + ((pix[0]-pix[1])<<16);
        a1 = (pix[2]+pix[3]) + ((pix[2]-pix[3])<<16);
        t[0] = a0 + a1;
        t[4] = a0 - a1;
        a2 = (pix[4]+pix[5]) + ((pix[4]-pix[5])<<16);
        a3 = (pix[6]+pix[7]) + ((pix[6]-pix[7])<<16);
        t[8] = a2 + a3;
        t[12] = a2 - a3;
    }
    for( int i = 0; i < 8; i++ )
    {
        HADAMARD4( a0, a1, a2, a3, tmp[i*4+0], tmp[i*4+1], tmp[i*4+2], tmp[i*4+3] );
        tmp[i*4+0] = a0;
        tmp[i*4+1] = a1;
        tmp[i*4+2] = a2;
        tmp[i*4+3] = a3;
        sum4 += abs2(a0) + abs2(a1) + abs2(a2) + abs2(a3);
    }
    for( int i = 0; i < 8; i++ )
    {
        HADAMARD4( a0,a1,a2,a3, tmp[i], tmp[8+i], tmp[16+i], tmp[24+i] );
        sum8 += abs2(a0) + abs2(a1) + abs2(a2) + abs2(a3);
    }
    dc = (uint16_t)(tmp[0] + tmp[8] + tmp[16] + tmp[24]);
    sum4 = (uint16_t)sum4 + ((uint32_t)sum4>>16) - dc;
    sum8 = (uint16_t)sum8 + ((uint32_t)sum8>>16) - dc;
    return ((uint64_t)sum8<<32) + sum4;
}

[RKSimon]

The only intrinsic calls I'm seeing are strided loads, which shouldn't go through the ICA changes in #124129 and #124254.

#123896 is a more likely candidate as the IR (19.x vs trunk) now contains a lot of <16 x i32> codegen, but I thought that was supposed to be NFC - @HanKuanChen any thoughts?

[alexey-bataev]

Looks like the cost model in TTI is too optimistic for some cases or misses/does not include some extra costs, which are spacemit-x60 microarchitecture specific

[alexey-bataev]

The only intrinsic calls I'm seeing are strided loads, which shouldn't go through the ICA changes in #124129 and #124254.

#123896 is a more likely candidate as the IR (19.x vs trunk) now contains a lot of <16 x i32> codegen, but I thought that was supposed to be NFC - @HanKuanChen any thoughts?

Ys, this is NFC actually. I don't see any major functional differences between 6e498bc and 26fc07d, maybe some sideffects, which triggered the extra vectorization?

[alexey-bataev]

I checked a bit, what's going on here. We have lots of extractelement instructions, which SLP then builds as a sequence of buildvector instructions. But later, InstCombiner replaces this buildvector with a whole series of shuffles, though (looks like) a buildvector is more profitable here for RISC-V.

[RKSimon]

Ideally InstCombine should leave this to VectorCombine where its more cost based, although it might still be missing some cases that InstCombine does manage.

[alexey-bataev]

Ideally InstCombine should leave this to VectorCombine where its more cost based, although it might still be missing some cases that InstCombine does manage.

Agree. But it also means that we're some effective lowering in codegen, most of these (ineffective) shuffles might be lowered the same way, through just a couple of vslideup/vslidedown