[PATCH] D125114: [SimplifyLibCalls] handle subobjects of constant aggregates

[Martin Sebor via Phabricator via llvm-commits]

msebor added a comment.

In D125114#3497477 <https://reviews.llvm.org/D125114#3497477>, @nikic wrote:

> In terms of high-level design, converting the initializer to a ConstantDataArray is not great: This is a very inefficient representation, especially if you take into account that these "temporary" ConstantDataArrays are globally interned and will probably never get freed.

I agree that the conversion is less than optimal.  The proposed approach is the least intrusive one I could come up with.  Everything else I considered would have required more extensive changes.  I'm interested in improving it but I'd prefer to do it incrementally rather than in the initial revision (or in a patch series).  
If I missed something please let me know.   If we want to improve this I'm not sure that starting by rearchitecting the existing design of these classes is a better approach than starting with this simple enhancement.  But I also don't know enough LLVM memory management to judge the costs of the overhead.

In D125114#3497580 <https://reviews.llvm.org/D125114#3497580>, @nikic wrote:

> In D125114#3497477 <https://reviews.llvm.org/D125114#3497477>, @nikic wrote:
>
>> Now that the ConstantDataArraySlice no longer corresponds to an already existing ConstantDataArray, it should probably be switched to store an array of bytes instead. From a cursory look, I don't think anything fundamentally depends on the current representation.
>
> Hm, I guess the need to represent slices of different element types could make things somewhat awkward. ConstantDataArray represents everything as an array of bytes internally, and then type-puns to the actual element type on access.

My initial thought was to store the byte representation but I ended up giving up on the idea because of that, at least for the initial patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125114