[PATCH] D122686: [InstCombine] Fold calls to strnlen (PR46455)

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:747
+      return B.CreateBinaryIntrinsic(Intrinsic::umin, Res, Bound);
+    };
   }
----------------
nikic wrote:
> msebor wrote:
> > nikic wrote:
> > > This is redundant: You already computed the bounded string lengths for both branches above, so there should be no need to bound the result here again.
> > I agree the `umin` is not necessary.  At the same time, removing it makes the code look like it's missing.  It should get folded away, but I don't have a strong preference one way or the other.
> The alternative here would be to first always calculate the unbounded string length and then do the umin once at the end. Actually, I think that would be cleaner than the current code structure, because most of the logic would be independent of the bound, and we'd only have to insert the umin at one place at the end, rather than in each branch of the string length computation.
I suppose that would be fine too.  Switching between `strlen` and `strnlen` computations in the same recursive call assumes there's no material difference between them elsewhere.  Ultimately, there shouldn't be, but as it is, doing as you propose bypasses the constraint of putting the "dynamic GEP handling behind a !Bound check" that you suggested to temporarily put in place.


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

https://reviews.llvm.org/D122686