[PATCH] D138874: [InstCombine] canonicalize trunc + insert as bitcast + shuffle, part 3

Wed Dec 7 10:44:12 PST 2022

spatel added a comment.

In D138874#3978809 <https://reviews.llvm.org/D138874#3978809>, @dmgreen wrote:

>> The good news is that potential regressions like above have been in main for almost a week now, and I haven't seen any bug reports/complaints yet.
>> So maybe this kind of IR pattern doesn't happen much in real code where it would be noticed.
>
> I had regressions reported from 2 places from those patches. I was looking into fixing those in the backend using a combine of `splat(bitcast(buildvector` or `splat(bitcast(scalar_to_vector` to `splat` (it was having trouble getting the buildvector legal types correct). Transforming trunc+insert to bitcast+shuffle feels like a bit of a strange canonicalization to me. We can probably fix it up in the backend (and the only regressions I've seen have both been unsimplified splats), but trunc+insert seems simpler.

Thanks for the update - so there has been some fallout.

I agree that trunc+insert is simpler in the basic case. The challenge is that we haven't found any other way to solve the motivating bug. Leaving it to the backend is too late, so we need to convert a chain of inserts into a shuffle in IR to get the optimal result.

This line of patches got us at least partially there. If we convert back to insert at SDAG builder/combine time, that seems like it could mitigate the problems. If that's not feasible, then I think we should revert the preceding patches in this set.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138874/new/

https://reviews.llvm.org/D138874