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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.
Herald added a subscriber: jsji.

In D125114#3523078 <https://reviews.llvm.org/D125114#3523078>, @msebor wrote:

> 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.

Yeah, I think we can start with this patch.



================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:650
+  TypeSize InitSize = DL.getTypeAllocSize(Init->getType());
+  if (InitSize < Offset)
+    return nullptr;
----------------
I think this is going to assert for a scalable vector global (unusual, but legal). You should probably just bail out in that case.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:651
+  if (InitSize < Offset)
+    return nullptr;
+
----------------
Do we need to protect against large globals here? I'm thinking about something like `{ i8 1, [100000000 x i8] zeroinitializer }`, which has a compact representation, but will be materialized in its full size here.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4171
+      uint64_t InitElSize = DL.getTypeStoreSize(InitElTy).getFixedSize();
+      if (InitElSize == ElementSize / 8)
+        // If Init is an initializer for an array of the expected size,
----------------
Is the type store size really what you want to check here? This means a) for ElementSize==32 this would accept a `float` array and b) for ElementSize==8 it would accept an `i4` array.

Now, this will actually get rejected by the isIntegerTy() check below afterwards, but it seems odd to first do a different check here. (This also means we should miss the ReadByteArrayFromGlobal code path for cases where it disagrees).


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4109
   // Look through bitcast instructions and geps.
   V = V->stripPointerCasts();
 
----------------
msebor wrote:
> nikic wrote:
> > You can replace this with stripAndAccumulateOffsets(), in which case you don't need explicit handling for GEPs.
> You mean replace the line above and the whole `if` statement below with a call to `stripAndAccumulateOffsets()` plus the handling of the resulting offset?  To do that I need to get a `DataLayout` reference from the `GlobalVariable` that `V` refers to, and I don't see how to do that without first stripping the intervening pointer casts.
You'd normally just pass in `const DataLayout &DL` as an argument. Happy to have that done as a followup though, to reduce API churn in this patch.


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

https://reviews.llvm.org/D125114