[PATCH] D88788: [SROA] rewritePartition()/findCommonType(): if uses have conflicting type, try getTypePartition() before falling back to largest integral use type (PR47592)

[Nuno Lopes via Phabricator via llvm-commits]

nlopes added a comment.

In D88788#2313981 <https://reviews.llvm.org/D88788#2313981>, @lebedev.ri wrote:

> In D88788#2313958 <https://reviews.llvm.org/D88788#2313958>, @nlopes wrote:
>
>> In D88788#2311004 <https://reviews.llvm.org/D88788#2311004>, @efriedma wrote:
>>
>>> I guess if we're relying on the allocated type of the alloca anyway, preferring it over an integer type isn't terrible.
>>>
>>> Really, though, we should avoid relying on the allocated type where possible.  Here, we could check if any of the load/store operations use a pointer type, and choose a pointer type in that case.
>>
>> Agreed. But until LLVM removes pointer sub-types it's convenient to get the alloca type right to avoid bitcast on every access anyway.
>> When pointer sub-types goes away, I guess all this code in SROA to find the right type for alloca would go away, but as you say it would have to be replaced with code to get the right load/store type instead. (FWIW Alive2's alloca only takes the number of bytes to allocate as argument)
>> So I see this patch as a step in the right direction.
>
> So i don't do this blindly to find out that is'a bad idea, can we agree on the baseline here?
> How should this be done properly? Instead of relying on the allocation type, would the D88842 <https://reviews.llvm.org/D88842>'s approach be applicable here?

I don't know SROA's code in detail to answer. I guess the point is that we don't want to introduce type-punning loads. So if the original program did a pointer load, the optimized program should do the same pointer load. Even if that means we can't combine 2 loads from the same address because one is pointer and another int. (see below the byte type proposal, as it allows you to combine loads into byte loads and then do a cast).

> I've looked now that rGe00f189d392d <https://reviews.llvm.org/rGe00f189d392dd9bf95f6a98f05f2d341d06cd65c> is done, and i see two-ish main origin sources of introduction inttoptr/ptrtoint that weren't there before:
>
> 1. instcombine's `SimplifyAnyMemTransfer()` - given `memcpy` of a pointer-sized thingy, we lower it to a load+store of an integer

This one requires a new `byte` type. We need it to support C's char anyway. Byte type doesn't have to be 1 byte, so maybe let's do it like integers: `b1, b2, b3, etc`.
Value with byte type could hold a value (or part of one) of any other LLVM type. No arithmetic operations are allowed on these types, only load/store and non-type punning type casts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88788