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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:650
+  TypeSize InitSize = DL.getTypeAllocSize(Init->getType());
+  if (InitSize < Offset)
+    return nullptr;
----------------
msebor wrote:
> 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?
Ah, I was just wrong about this, sorry. We have scalable `Constant`s, but if they can't be used as a GV initializer, then there's no problem as far as this code is concerned.


================
Comment at: llvm/lib/Analysis/ConstantFolding.cpp:651
+  if (InitSize < Offset)
+    return nullptr;
+
----------------
msebor wrote:
> 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.
The closest thing I could find is instcombine-maxarray-size with value 1024, which might be a bit low for this usage. I'd probably pick 65536 as a nice round value, but I don't care strongly about this limit -- it's a theoretical concern at this point, and we can always adjust it.


================
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,
----------------
msebor wrote:
> 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.
Okay, I see now that ConstantDataArray only accepts power-of-2, multiple of 8 types, so arrays of `i4` and similar don't take this codepath. An example for the `float` issue is this:
```
declare i64 @wcslen(ptr)

@g = constant [3 x float] [float 1.0, float 2.0, float 0.0]

define i64 @test() {
  %res = call i64 @wcslen(ptr @g)
  ret i64 %res
}

!llvm.module.flags = !{!0}
!0 = !{i32 1, !"wchar_size", i32 4}
```
This will assert with the current patch.

The initializer can only be directly reused if `isIntegerTy(ElementSize)`, in other cases we should go through the `ReadByteArrayFromGlobal()` code to convert the elements into integers (even if they already have the correct size -- the consuming code expects that they are integers).

We could change the API contract to allow non-integer elements to be returned, but I don't think that would be useful for any current users of this API.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4257
+        Array = ArrayInit;
+	ArrayTy = ArrayInit->getType();
+      }
----------------
Something is broken with the indentation here and below.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4274
+      else
+	ArrayTy = dyn_cast<ArrayType>(Init->getType());
+
----------------
Just `ArrayTy = cast<ArrayType>(Init->getType())` should be sufficient for both cases -- ReadByteArrayFromGlobal() should always return a constant with array type.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1297
+			       /*TrimAtNul=*/false) ||
+	// TODO: Handle zeroinitializer.
+	!StopChar)
----------------
Broken indentation here as well.


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

https://reviews.llvm.org/D125114