[PATCH] D135263: [ValueTracking][SimplifyLibCalls] Fix bug in getConstantDataArrayInfo for wchar_t

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope created this revision.
bjope added a reviewer: MatzeB.
Herald added a subscriber: hiraditya.
Herald added a project: All.
bjope requested review of this revision.
Herald added a project: LLVM.

When SimplifyLibCalls is dealing with wchar_t (e.g. optimizing wcslen)
it uses ValueTracking helpers with a CharSize/ElementSize that isn't
8, but rather 16 or 32 (to match with the size in bits of a wchar_t).

Problem I've seen is that llvm::getConstantDataArrayInfo is taking
both a "ElementSize" argument (basically indicating size of a
char/element in bits) and an "Offset" which afaict is an offset
in the unit "number of elements". Then it also use
stripAndAccumulateConstantOffsets to get a "StartIdx" which afaict
is calculated in bytes. The returned Slice.Length is based on
arithmetics that add/subtract variables that are having different
units (bytes vs elements). Most notably I think the "StartIdx" must
be scaled using the "ElementSize" to get correct results.

The symptom of the above problem can be seen in the wcslen-7.ll test
case which miscompiles.

This patch is supposed to resolve the bug by converting between
bytes and elements when needed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D135263

Files:
  llvm/lib/Analysis/ValueTracking.cpp
  llvm/test/Transforms/InstCombine/wcslen-7.ll


Index: llvm/test/Transforms/InstCombine/wcslen-7.ll
===================================================================
--- llvm/test/Transforms/InstCombine/wcslen-7.ll
+++ llvm/test/Transforms/InstCombine/wcslen-7.ll
@@ -11,10 +11,9 @@
 
 
 ; Fold wcslen(ws + 2) => 7.
-; FIXME: This fold is faulty, result should be 7 not 1.
 define dso_local i64 @fold_wcslen_1() {
 ; CHECK-LABEL: @fold_wcslen_1(
-; CHECK-NEXT:    ret i64 1
+; CHECK-NEXT:    ret i64 7
 ;
   %ps3_pi = getelementptr inbounds [10 x i32], ptr @ws, i64 0, i64 2
   %len = tail call i64 @wcslen(ptr %ps3_pi)
Index: llvm/lib/Analysis/ValueTracking.cpp
===================================================================
--- llvm/lib/Analysis/ValueTracking.cpp
+++ llvm/lib/Analysis/ValueTracking.cpp
@@ -4232,10 +4232,14 @@
 // its initializer if the size of its elements equals ElementSize, or,
 // for ElementSize == 8, to its representation as an array of unsiged
 // char. Return true on success.
+// Offset is in the unit "nr of ElementSize sized elements".
 bool llvm::getConstantDataArrayInfo(const Value *V,
                                     ConstantDataArraySlice &Slice,
                                     unsigned ElementSize, uint64_t Offset) {
-  assert(V);
+  assert(V && "V should not be null.");
+  assert((ElementSize % 8) == 0 &&
+         "ElementSize expected to be a multiple of the size of a byte.");
+  unsigned ElementSizeInBytes = ElementSize / 8;
 
   // Drill down into the pointer expression V, ignoring any intervening
   // casts, and determine the identity of the object it references along
@@ -4259,7 +4263,11 @@
     // Fail if the constant offset is excessive.
     return false;
 
-  Offset += StartIdx;
+  // Off/StartIdx is in the unit of bytes. So we need to convert to number of
+  // elements.
+  assert((StartIdx % ElementSizeInBytes) == 0 &&
+         "Constant offset not a multiple of the ElementSize.");
+  Offset += StartIdx / ElementSizeInBytes;
 
   ConstantDataArray *Array = nullptr;
   ArrayType *ArrayTy = nullptr;
@@ -4267,7 +4275,7 @@
   if (GV->getInitializer()->isNullValue()) {
     Type *GVTy = GV->getValueType();
     uint64_t SizeInBytes = DL.getTypeStoreSize(GVTy).getFixedSize();
-    uint64_t Length = SizeInBytes / (ElementSize / 8);
+    uint64_t Length = SizeInBytes / ElementSizeInBytes;
 
     Slice.Array = nullptr;
     Slice.Offset = 0;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D135263.465358.patch
Type: text/x-patch
Size: 2379 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20221005/f5a2f444/attachment.bin>