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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:302
+  Value *CharVal = CI->getArgOperand(1);
+  ConstantInt *CharC = dyn_cast<ConstantInt>(CharVal);
   if (!CharC) {
----------------
Unrelated change (in strchr rather than memchr)?


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1055
+      EndOff = Str.size();
+  }
+
----------------
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.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1066
+    //   N != 0 ? (S[0] == C ? S : S[Pos] == C ? S + Pos : null) : null
+    // where       ^Sel2    and    ^Sel1 are denoted above.
+    // The latter makes it also possible to fold strchr() calls with
----------------
I don't think this is correct for the case where `N` is variable. Let's say we have `S = "12"`, `C = 2` and `N = 1` (known only at runtime). The correct result for this is `null`. I believe your code would return `S + 1`.

There doesn't seem to be test coverage for this case (variable N with two runs -- there's only one with three runs).


================
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]]
+;
----------------
Why does this one not fold?


================
Comment at: llvm/test/Transforms/InstCombine/memchr-6.ll:53
+; CHECK-NEXT:    [[DOTNOT:%.*]] = icmp eq i64 [[N:%.*]], 0
+; CHECK-NEXT:    [[MEMCHR_SEL3:%.*]] = select i1 [[DOTNOT]], i8* null, i8* [[MEMCHR_SEL2]]
+; CHECK-NEXT:    ret i8* [[MEMCHR_SEL3]]
----------------
Unrelated: There's a canonicalization opportunity here to convert `C1 ? X : (C2 ? X : Y) to `(C1 ? true : C2) ? X : Y` (or vice versa, but I'd consider logical or more canonical here).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126515