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

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4257
+        Array = ArrayInit;
+	ArrayTy = ArrayInit->getType();
+      }
----------------
nikic wrote:
> Something is broken with the indentation here and below.
Tabs (required by GCC) vs spaces (LLVM).


================
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:
> 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.
I see what you mean now, thanks for the test case!  This is a limitation of the current approach of restricting conversions to arrays of `i8`.  The only existing use case would be the `wcslen` folder so that alone is probably not worth an API change.  But as we discussed the conversion from the underlying byte representation to a byte array is inefficient and avoiding it would also make it possible to extend the API to support arbitrary conversions.  I'll keep that in mind for a possible follow up.


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

https://reviews.llvm.org/D125114