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

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:650
+  TypeSize InitSize = DL.getTypeAllocSize(Init->getType());
+  if (InitSize < Offset)
+    return nullptr;
----------------
nikic wrote:
> I think this is going to assert for a scalable vector global (unusual, but legal). You should probably just bail out in that case.
I found the following in `llvm/test/Verifier/scalable-global-vars.ll` while looking for a way to define such a vector:

```
;; Global variables cannot be scalable vectors, since we don't
;; know the size at compile time.

; CHECK: Globals cannot contain scalable vectors
; CHECK-NEXT: <vscale x 4 x i32>* @ScalableVecGlobal
@ScalableVecGlobal = global <vscale x 4 x i32> zeroinitializer
```
Are there other scalable vectors that can be defined at global scope?


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:651
+  if (InitSize < Offset)
+    return nullptr;
+
----------------
nikic wrote:
> 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.
It might make sense to cap the size (at a minimum to avoid the 32 vs 64-bit `size_t` problem).  Otherwise the `SmallVector`ctor aborts when `malloc` fails.  I couldn't find code to follow as an example to prevent this so I sort of arbitrarily picked `INT_MAX` as the maximum.  I've added `memchr-7.ll` to exercise this although I imagine the test could fail on memory constrained systems.  Let me know if there's a guideline for how to choose a more suitable maximum in these situations or how to write a more robust test.


================
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,
----------------
nikic wrote:
> 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).
I'm not sure I understand the question but matching sizes is the purpose of the test and of the `ElementSize` argument.  The point of the `isIntegerTy()` check seems to be to prevent folding `wcslen` calls with zero-initialized arrays with a smaller size.  Since such calls are undefined the only benefit of going out of our way and expanding them to the library call that I can think of is to get them caught by the sanitizer.  I've replaced the check for type with one for size.


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

https://reviews.llvm.org/D125114