[clang] [llvm] [SROA] Canonicalize homogeneous structs into fixed vectors (PR #165159)

[Yonah Goldberg via llvm-commits]

https://github.com/YonahGoldberg commented:

My overall feelings are that the heuristic used in `shouldCanonicalizeHomogeneousStructToVector` is overly complex and possibly over-fitted to the llvm-opt-benchmark set. For example, the heuristics on lines 5263 to 5273 seem pretty arbitrary. Why only allow i64 non-whole-alloca memcpys to get transformed? And then why do we disallow canonicalization if there is a sub-element load, but not a sub-element store?

My intuition is the effect you're trying to have here is that you don't want this transformation to result in any `insertelement` or `extractelement` instructions. Is that right? So I thought we could do something much simpler, like say that we can canonicalize if:

1. All the users of the partition are `MemInst`
2. Or all the users are whole loads/stores to the entire partition.

I checked this simpler heuristic against two of your lit tests and found some interesting results:

1. For struct-to-vector-subpartition.ll, this simpler heuristic removes the `alloca { i64, i64 }`. This would get removed later anyway by `memcpyopt`, but I still see this as a win.
2. For struct-to-vector-fp-store-only-tail, this test case is much more interesting. If you're allowed to canonicalize the [0,16] partition to <4 x float> and then you run instcombine, you end up with:

```
define void @store_only_fp_tail(ptr noalias %dst) {
  store <4 x float> <float 0.000000e+00, float undef, float undef, float undef>, ptr %dst, align 1
  %.sroa.3.0.dst.sroa_idx = getelementptr inbounds nuw i8, ptr %dst, i64 16
  store float 0.000000e+00, ptr %.sroa.3.0.dst.sroa_idx, align 1
  ret void
}
```

And then memcpyopt optimizes to:
```
define void @store_only_fp_tail(ptr noalias writeonly captures(none) initializes((0, 20)) %dst) local_unnamed_addr #0 {
  tail call void @llvm.memset.p0.i64(ptr noundef nonnull align 1 dereferenceable(20) %dst, i8 0, i64 20, i1 false)
  ret void
}
```

In comparison, with your heuristic, we disallow the canonicalization and get:
```
define void @store_only_fp_tail(ptr noalias %dst) {
  store float 0.000000e+00, ptr %dst, align 1
  %.sroa.33.0.dst.sroa_idx = getelementptr inbounds nuw i8, ptr %dst, i64 16
  store float 0.000000e+00, ptr %.sroa.33.0.dst.sroa_idx, align 1
  ret void
}
```

So the bug looks like memcpyopt is initializing previously uninitialized memory, so you end up with a wider store, which translates to additional instructions on some backends. I remember finding a similar bug a while back--I wonder if there should be some instcombine transformation that transforms the store <4 x float> to a scalar store? Or should memcpyopt handle this?

Maybe you already saw this and there's a reason you went for a more complicated heuristic. Would be happy to get on a call sometime next week to talk about it if you want because this is getting quite complicated.

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