[AArch64] LowerBUILD_VECTOR contains an apparently unreachable DUPLANE path
- Dominant language
- LLVM
- Stars
- 40.5k
- Forks
- 18.7k
- PR merge metrics
- PR metrics pending
Description
## Summary
While investigating #221122 and working on #221186, I noticed an apparently
unreachable `DUPLANE` lowering path in
`AArch64TargetLowering::LowerBUILD_VECTOR()`.
This appears to be independent of the correctness issue originally being
investigated.
## Details
For a non-constant splat, `LowerBUILD_VECTOR()` currently contains:
```cpp
if (usesOnlyOneValue) {
if (!isConstant) {
if (Value.getOpcode() != ISD::EXTRACT_VECTOR_ELT ||
Value.getValueType() != VT) {
LLVM_DEBUG(
dbgs() << "LowerBUILD_VECTOR: use DUP for non-constant splats\n");
return DAG.getNode(AArch64ISD::DUP, DL, VT, Value);
}
// This is actually a DUPLANExx operation, which keeps everything vectory.
SDValue Lane = Value.getOperand(1);
Value = Value.getOperand(0);
if (Value.getValueSizeInBits() == 64) {
LLVM_DEBUG(
dbgs() << "LowerBUILD_VECTOR: DUPLANE works on 128-bit vectors, "
"widening it\n");
Value = WidenVector(Value, DAG);
}
unsigned Opcode = getDUPLANEOp(VT.getVectorElementType());
return DAG.getNode(Opcode, DL, VT, Value, Lane);
}
}
```
Here, `VT` is the result type of the `BUILD_VECTOR`, so it is a vector type,
for example:
```text
v4i16
```
`Value`, however, is one of the scalar operands of the `BUILD_VECTOR`. Its
type is therefore the vector element type, or a promoted scalar integer type,
for example:
```text
i16
```
or:
```text
i32
```
This means the check:
```cpp
Value.getValueType() != VT
```
appears to always be true for a valid `BUILD_VECTOR`.
As a result, the function returns the scalar-fed `AArch64ISD::DUP` before the
following `DUPLANE` path can be reached.
## History
`git blame` shows that the structure of this code, including the type
comparison and the following `DUPLANE` path, dates back to the initial ARM64
backend import:
```text
00ed9964c659 ARM64: initial backend import
```
So this does not appear to be a recent regression.
## Existing DUP lane lowering
Normal extract-and-splat cases are already lowered efficiently elsewhere.
For example:
```llvm
define <4 x i16> @splat_i16(<8 x i16> %v) {
entry:
%x = extractelement <8 x i16> %v, i64 3
%r0 = insertelement <4 x i16> poison, i16 %x, i64 0
%r = shufflevector <4 x i16> %r0, <4 x i16> poison,
<4 x i32> zeroinitializer
ret <4 x i16> %r
}
```
currently produces:
```asm
dup v0.4h, v0.h[3]
ret
```
Equivalent `i8` and `i32` cases also produce direct vector-lane `dup`
instructions.
The AArch64 backend has other combine logic for recognizing
`DUP(EXTRACT_VECTOR_ELT(...))`, so removing the apparently unreachable block
from `LowerBUILD_VECTOR()` may simply be an NFC cleanup.
## Question
Is there a case in which the `DUPLANE` block following
`Value.getValueType() != VT` can currently be reached, or can this stale path
be removed as an NFC cleanup?
If it is confirmed to be unreachable, I can prepare a cleanup patch.
Contributor guide
Research direction
Start in AArch64TargetLowering::LowerBUILD_VECTOR() and trace the types of BUILD_VECTOR operands through the non-constant splat path. Compare this with the existing DUP(EXTRACT_VECTOR_ELT(...)) combine logic and the supplied AArch64 examples. Done means confirming whether the DUPLANE block is reachable and, if not, removing it as an NFC cleanup.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp
- Domain
- compilers
- Issue type
- Refactor
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100