[PATCH] D126515: [InstCombine] Fold memchr of sequences of same characters

[Nikita Popov via Phabricator via llvm-commits]

nikic accepted this revision.
nikic added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1055
+      EndOff = Str.size();
+  }
+
----------------
msebor wrote:
> nikic wrote:
> > As far as I can tell EndOff doesn't actually get used later, it's only used for the substr(), so I'm not sure why this code is changing rather than only being moved.
> > 
> > I think in https://reviews.llvm.org/D123472 the wrong version of the patch ended up being committed, I remember there being a static substr() function for this purpose, but it doesn't seem to be present on main.
> Yes, I must have committed a prior version of the patch.
I'd suggest landing the substr() function as a separate NFC patch, as it's not directly related to this one (and we'd want to replace the other usages at the same time as well).


================
Comment at: llvm/test/Transforms/InstCombine/memchr-6.ll:19
+; CHECK-NEXT:    [[RET:%.*]] = call i8* @memchr(i8* noundef nonnull dereferenceable(1) getelementptr inbounds ([5 x i8], [5 x i8]* @a00000, i64 0, i64 0), i32 [[C:%.*]], i64 5)
+; CHECK-NEXT:    ret i8* [[RET]]
+;
----------------
msebor wrote:
> nikic wrote:
> > Why does this one not fold?
> Because `a00000` has no initializer `getConstantDataArrayInfo` returns a null `Slice.Array` that `getConstantStringInfo` then fails for.  I've added a comment to the test.
> 
> I thought I'd handled it D125114 but apparently it needs another enhancement.  I can take care of it in a followup.
Hrm, that would probably also require an API change, because we don't have any already allocated string we could create a StringRef to.


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

https://reviews.llvm.org/D126515