llvm / llvm/llvm-project

[VectorCombine][X86] old-cost model overestimates `load + insertelement poison` patterns on x86

Open
#206,446 2 comments 0 reactions 1 assignee Claimed by @RKSimon View on GitHub
backend:X86
Dominant language
LLVM
Stars
40.5k
Forks
18.7k
PR merge metrics
PR metrics pending

Description

### Summary

While investigating the `NeedCast` path in `VectorCombine::vectorizeLoadInsert`, I noticed that the old-cost calculation can overestimate the cost of the original IR pattern:

```llvm
%s = load i32, ptr %gep, align 1
%r = insertelement <4 x i32> poison, i32 %s, i64 0
```

On x86, this pattern can lower to a single scalar memory load into a vector register, for example:

```asm
vmovss xmm0, dword ptr [rdi + 5]
ret
```

However, the current VectorCombine cost model charges both the scalar load cost and a scalarization/insert overhead:

```text
OldLoadCost: 1
OldScalarizationCost: 1
OldCost: 2
```

This makes the original form look more expensive than it actually is.

### Reproducer

```llvm
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"

define <4 x i32> @gep05_bitcast_load_i32_from_v8i16_insert_v4i32(ptr align 1 dereferenceable(16) %p) {
%gep = getelementptr inbounds <8 x i16>, ptr %p, i64 0, i64 5
%s = load i32, ptr %gep, align 1
%r = insertelement <4 x i32> poison, i32 %s, i64 0
ret <4 x i32> %r
}
```

A corresponding transformed form is:

```llvm
define <4 x i32> @candidate(ptr align 1 dereferenceable(16) %p) {
%wide = load <8 x i16>, ptr %p, align 1
%shuf = shufflevector <8 x i16> %wide, <8 x i16> poison,
<8 x i32>
%r = bitcast <8 x i16> %shuf to <4 x i32>
ret <4 x i32> %r
}
```

On x86 with AVX, the original form lowers to:

```asm
vmovss xmm0, dword ptr [rdi + 5]
ret
```

The transformed form lowers to something like:

```asm
vmovdqu xmm0, xmmword ptr [rdi]
vpsrldq xmm0, xmm0, 10
ret
```

So the original code is effectively one instruction, while the transformed code is a vector load plus a byte shift.

### Cost model observation

I added debug logging around the old-cost calculation in `VectorCombine::vectorizeLoadInsert`:

```text
VC load-insert old cost:
LoadTy: i32
VecTy: <4 x i32>
DemandedElts: 1
HasExtract: 0
OldLoadCost: 1
OldScalarizationCost: 1
OldCost: 2

VC load-insert cost:
NeedCast: 1
LoadTy: i32
MinVecTy: <8 x i16>
ResultTy: <4 x i32>
OldCost: 2 vs NewCost: 2
```

The old cost is computed as:

```cpp
OldCost = TTI.getMemoryOpCost(... load i32 ...);

OldCost += TTI.getScalarizationOverhead(VecTy, DemandedElts,
/*Insert=*/true,
HasExtract,
CostKind);
```

I also checked whether passing `ForPoisonSrc=true` to `getScalarizationOverhead()` fixes the estimate, but the scalarization overhead still does not drop to zero for this case.

### Problem

The issue appears to be that `getScalarizationOverhead()` models the cost of building a vector from scalar values. That is reasonable for a pattern like:

```llvm
%x = some scalar value
%r = insertelement <4 x i32> poison, i32 %x, i64 0
```

However, the old form in `vectorizeLoadInsert` is more specific:

```llvm
%s = load i32, ptr %gep, align 1
%r = insertelement <4 x i32> poison, i32 %s, i64 0
```

On x86, this can be selected as a single memory load directly into the vector register. In other words, the scalar load and the lane-0 insertion are not two separate costs in the final code.

The current cost model therefore overestimates the old form:

```text
model:
scalar load cost + insert/scalarization overhead = 2

actual x86 lowering:
scalar memory load into vector register = 1
```

This can make VectorCombine accept transforms that are not actually profitable.

### Expected behavior

For the specific old pattern:

```llvm
insertelement poison, (one-use load T), 0
```

the cost model should not blindly add a scalarization/insert overhead if the target can lower the original form as a single memory load into a vector register.

### Possible directions

There are a few possible ways to address this:

1. Teach `getScalarizationOverhead()` / target TTI to account for this kind of memory-source insertion pattern.
2. Extend the cost query so the caller can express that the inserted scalar value comes directly from a load.
3. Handle this specifically at the `VectorCombine::vectorizeLoadInsert` call site, since that transform already knows it is comparing a `load + insertelement` compound pattern.

I am not sure which direction is preferred. The important point is that the current use of `getScalarizationOverhead()` in this call site does not accurately model the original x86 lowering for `load + insertelement poison, lane 0`.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.