[llvm] [X86][GlobalIsel] Support G_BUILD_VECTOR for non constant using G_INSERT_VECTOR_ELT (PR #204078)

[via llvm-commits]

mahesh-attarde wrote:


## Issue 1: Float build_vector uses GPR round-trip instead of INSERTPS/VINSERTPS

**Affected functions:** `build_vector_v4f32`, `build_vector_v8f32`, `build_vector_v16f32`

**GISEL output (v4f32 AVX):**
```asm
vmovd %xmm0, %eax        ; XMM -> GPR
vmovd %eax, %xmm0        ; GPR -> XMM (movd)
vmovd %xmm1, %eax        ; XMM -> GPR
vpinsrd $1, %eax, %xmm0  ; GPR -> insert as integer
...
```

**SDAG output (v4f32 AVX):**
```asm
vinsertps $0x10, %xmm1, %xmm0, %xmm0   ; direct XMM-to-XMM insert
vinsertps $0x20, %xmm2, %xmm0, %xmm0
vinsertps $0x30, %xmm3, %xmm0, %xmm0
```

**Root cause:**
The GISEL legalization of `G_BUILD_VECTOR` for `v4f32` lowers to
`G_INSERT_VECTOR_ELT` with the element typed as `s32` (line 836 in
`X86LegalizerInfo.cpp`). The register bank allocator puts float scalars
(`fr32`) in the VECR bank, but `G_INSERT_VECTOR_ELT` for 32-bit elements
selects to `VPINSRDrri` which requires a GPR source operand. This forces
a `COPY fr32 -> gr32` (vmovd xmm->eax) followed by the pinsrd.

SDAG recognizes the float-typed build_vector and uses `INSERTPS`/`VINSERTPS`
which takes an XMM source directly, avoiding the GPR domain crossing.

**Fix approach:**
- Option A: In `selectInsertVectorElt`, detect when the element source is an
  `fr32` (VECR bank) and select `INSERTPSrr`/`VINSERTPSrr` instead of
  `VPINSRDrri` for 32-bit float elements.
- Option B: Add a combine or custom legalization that recognizes
  `G_BUILD_VECTOR <v4 x s32>` where all sources originate from float registers
  and directly emits `G_INSERTPS` (a new X86-specific generic opcode) or lowers
  differently.
- Option C: Teach the regbank allocator to map `G_INSERT_VECTOR_ELT` element
  source to VECR when input is float, and add INSERTPS selection path.

**Priority:** High - this is 2x the instruction count for v4f32 and causes
unnecessary cross-domain penalties.


## Issue 2: Redundant `vinsertf128 $0` / `vinserti32x4 $0` for low sub-vector

**Affected functions:** `build_vector_v8i32`, `build_vector_v4i64`,
`build_vector_v8f32`, `build_vector_v4f64`, `build_vector_v16i32`,
`build_vector_v8i64`, `build_vector_v16f32`, `build_vector_v8f64`

**GISEL output (v8i32):**
```asm
vmovd %edi, %xmm0
vpinsrd $1, %esi, %xmm0, %xmm0
vpinsrd $2, %edx, %xmm0, %xmm0
vpinsrd $3, %ecx, %xmm0, %xmm0
vinsertf128 $0, %xmm0, %ymm0, %ymm0   ; <-- redundant! xmm0 IS low ymm0
vmovd %r8d, %xmm1
...
```

**SDAG output (v8i32):**
```asm
vmovd %edi, %xmm0
vpinsrd $1, %esi, %xmm0, %xmm0
vpinsrd $2, %edx, %xmm0, %xmm0
vpinsrd $3, %ecx, %xmm0, %xmm0
                                       ; no insert for sub-vector 0
vmovd %r8d, %xmm1
...
```

**Root cause:**
In `legalizeBuildVector` (line 806-823 of `X86LegalizerInfo.cpp`), the code
does:
```cpp
Register Vec = MIRBuilder.buildUndef(DstTy).getReg(0);
for (unsigned Sub = 0; Sub < NumSubVecs; ++Sub) {
    // build 128-bit sub-vector...
    if (HasNonUndef)
        Vec = MIRBuilder.buildInsert(DstTy, Vec, SubVec, Sub * 128).getReg(0);
}
```
When `Sub == 0`, it emits `G_INSERT ... offset=0` into an undef vector.
This is semantically a no-op (inserting a 128-bit value at offset 0 of a
256/512-bit undef is equivalent to just widening the 128-bit value) but gets
selected to a real `VINSERTF128 $0` instruction.

SDAG avoids this because its BUILD_VECTOR lowering directly constructs the
low half as the base and only inserts the upper halves.

**Fix approach:**
- Skip the `buildInsert` when `Sub == 0` (the Vec is still IMPLICIT_DEF, so
  inserting at offset 0 just defines the low lane). Instead, widen the sub-vector
  to the destination type using SUBREG_TO_REG / INSERT_SUBREG semantics.
- Alternatively, add a post-legalization combine that folds
  `G_INSERT(G_IMPLICIT_DEF, val, 0)` into a sub-register definition.

**Priority:** Medium - one extra instruction per wide build_vector, but it may
be eliminated by the hardware's register renaming in some cases.


## Issue 3: GISEL uses per-128-bit-lane `vinserti32x4` while SDAG uses hierarchical 256-bit inserts

**Affected functions:** `build_vector_v16i32`, `build_vector_v8i64`

**GISEL output (v16i32):**
```asm
vinserti32x4 $0, %xmm0, %zmm0, %zmm0
...
vinserti32x4 $1, %xmm1, %zmm0, %zmm0
...
vinserti32x4 $2, %xmm1, %zmm0, %zmm0
...
vinserti32x4 $3, %xmm1, %zmm0, %zmm0
```

**SDAG output (v16i32):**
```asm
; build low 128
; build next 128
vinserti128 $1, %xmm1, %ymm0, %ymm0   ; form 256-bit low half
; build next 128
; build next 128
vinserti128 $1, %xmm2, %ymm1, %ymm1   ; form 256-bit high half
vinserti64x4 $1, %ymm1, %zmm0, %zmm0  ; combine into 512-bit
```

**Root cause:**
`legalizeBuildVector` always splits into 128-bit chunks and inserts each one
directly into the 512-bit destination. SDAG uses a hierarchical approach:
build 128->256->512 using wider insert instructions. The hierarchical approach:
1. Avoids the redundant insert at offset 0.
2. Uses `vinserti128` (shorter encoding) for 256-bit halves.
3. Uses `vinserti64x4` for the final 512-bit combine.

The flat approach generates 4x `vinserti32x4` (EVEX-encoded, 6 bytes each)
while the hierarchical approach generates `vinserti128` (VEX, 5 bytes) +
`vinserti128` + `vinserti64x4`.

**Fix approach:**
Modify `legalizeBuildVector` for VecSize > 256 to use a two-level strategy:
first build 256-bit halves (each from two 128-bit inserts), then combine with
a 256-bit insert. This matches SDAG's approach.

**Priority:** Low-medium - mostly an encoding size difference; performance impact
is minimal since all instructions are single-uop on modern CPUs.


## Issue 4: GISEL processes sub-vectors sequentially (lo-to-hi) while SDAG reorders for register pressure

**Affected functions:** `build_vector_v4i64`, `build_vector_v8i64`,
`build_vector_v4f64`, `build_vector_v8f64`

**GISEL output (v4i64):**
```asm
vmovq %rsi, %xmm0       ; build low pair first
vmovq %rdi, %xmm1
vpunpcklqdq xmm0 = xmm1[0],xmm0[0]
vinsertf128 $0, ...      ; insert low (redundant)
vmovq %rcx, %xmm1       ; then high pair
vmovq %rdx, %xmm2
vpunpcklqdq xmm1 = xmm2[0],xmm1[0]
vinsertf128 $1, ...      ; insert high
```

**SDAG output (v4i64):**
```asm
vmovq %rcx, %xmm0       ; build HIGH pair first
vmovq %rdx, %xmm1
vpunpcklqdq xmm0 = xmm1[0],xmm0[0]
vmovq %rsi, %xmm1       ; then low pair
vmovq %rdi, %xmm2
vpunpcklqdq xmm1 = xmm2[0],xmm1[0]
vinsertf128 $1, %xmm0, %ymm1, %ymm0   ; single insert
```

**Root cause:**
SDAG's scheduler reorders operations to minimize register pressure and avoid
the need for the `$0` insert. By building the high sub-vector first and
the low sub-vector second, the low sub-vector result naturally becomes the
base of the final `vinsertf128 $1` — no separate low-insert is needed.

GISEL processes lanes 0,1,2,3 in order, builds lane 0 first (requiring an
explicit insert at offset 0), then inserts lane 1 on top.

**Fix approach:**
This is partially addressed by fixing Issue 2 (removing the redundant $0
insert). The instruction ordering difference is cosmetic and doesn't affect
performance after the redundant insert is removed.

**Priority:** Low - mainly cosmetic after Issue 2 is fixed.



## Issue 5: GISEL uses VPUNPCKLQDQ for v2f64 build while SDAG uses VMOVLHPS

**Affected functions:** None currently mismatched for 128-bit (both use movlhps).
But for 256/512-bit `v4f64`/`v8f64`, GISEL correctly uses `vmovlhps` for the
f64 pair combine — this already matches SDAG.

*This was previously a mismatch but appears to be fixed by the
`insert_vec_elt_to_scalar_to_vec` combine and the PUNPCKLQDQ special-case in
`selectInsertVectorElt`.*

**Status:** RESOLVED - no action needed.




https://github.com/llvm/llvm-project/pull/204078