This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold calls to strnlen (PR46455)
AbandonedPublic

Authored by msebor on Mar 29 2022, 2:30 PM.

Details

Summary

This patch adds support for folding simple strnlen calls to SimplifyLibCalls.cpp. It extends the optimizeStringLength member function to accept the bound AKA maxlen argument to strnlen and attempt to fold calls to it analogously to strlen. To keep optimizeStringLength from growing unwieldy the patch factors out the string length logic into a new helper function, minStringLength, and adds most of the strnlen folding there.

The new minStringLength function can be called recursively to simplify some less than completely trivial calls with conditional expressions such as strnlen(x ? "123" : "12345" + i, n). In case one of the recursive calls fails, rather than creating and inserting the simplified instructions into the IR as it goes, the function returns a pointer to a function/lambda that does that. On success the caller then evaluates the lambda to create and insert the instructions (or constants) into the IR.

I have tested the patch by bootstrapping LLVM + Clang on x86_64-linux and running check-all.

If there are better solutions than the lambda approach above I'd be happy to learn about them and adjust the patch (and follow up on the question I posted about this on Discourse: https://discourse.llvm.org/t/61309).

The new minStringLength function can also be called to implement wcsnlen folding. I have done that in a separate patch that I'll submit if/when this is accepted.

https://github.com/llvm/llvm-project/issues/46455

Diff Detail

Event Timeline

msebor created this revision.Mar 29 2022, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 2:30 PM
msebor requested review of this revision.Mar 29 2022, 2:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 29 2022, 2:30 PM
lebedev.ri retitled this revision from Fold calls to strnlen (PR46455) to [InstCombine] Fold calls to strnlen (PR46455).Mar 29 2022, 2:36 PM
lebedev.ri edited the summary of this revision. (Show Details)
nikic added a comment.Mar 31 2022, 3:30 AM

It would be good to split this up into smaller parts where possible. Two obvious bits would be to precommit the tests with baseline checks (NFC) and land the addition of nonnull attributes on strnlen (which is trivial).

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
644

It would be fine to just emit the umin unconditionally here and let it fold away if it happens to be constant, something like:

Value *Min = ConstantInt::get(CI->getType(), Len - 1);
return [&]() {
  return Bound ? B.CreateBinaryIntrinsic(Intrinsic::umin, Min, Bound) : Min;
};
672

I had a bit of a hard time understanding this new special case. I'm wondering, if you have no null and use strnlen, can't we simply always return Bound as the result? Either the result will be Bound, or we're reading past the extent of the global and have UB. Or if we don't want to optimize the latter case we can keep the MaxLen < ArraySize check, but in either case I think mixing it with the code below is kind of confusing -- in particular the sub bit doesn't seem relevant?

710

If we moved handling of the isOnlyUsedInZeroEqualityComparison() check in optimizeStringLength() before the minStringLength() handling, could we drop the special check here?

768

Might be more elegant with m_ConstantInt, something like...

uint64_t MaxLen;
if (!Bound || !match(Bound, m_ConstantInt(MaxLen))
  MaxLen = UINT64_MAX;

if (MaxLen == 0)
  return ConstantInt::get(CI->getType(), 0);
777

Why does this need the MaxLen == UINT64_MAX check?

llvm/test/Transforms/InstCombine/strlen-4.ll
4

Please use llvm/utils/update_test_checks.py to generate InstCombine test checks.

llvm/test/Transforms/InstCombine/strnlen-5.ll
2

This test file could probably be called strnlen-icmp.ll or something? Just numbering them isn't super meaningful.

llvm/test/Transforms/InstCombine/strnlen-6.ll
53

This test output looks wrong?

Thanks for the review! Let me upload a new diff with the suggested changes. I'm not sure what the process is for submitting the baseline tests: do I need to open a separate review for those? Likewise for the nonnull attribute for strnlen? I'm also not sure I have commit rights yet so if/when any of this is approved someone might need to commit the changes for me.

Finally, the build bot shows unit test failures that I'm not sure I understand yet and could use some guidance with. Are ASan test timeouts expected? Any idea about the BOLT failures?

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
644

Done, thanks.

672

That's a good question. I was mostly trying to follow the strlen handling here. But if there's no concern with the unterminated array being followed by another that does have a nul I'm okay with your suggestion. The specific case I have in mind is an aggregate with two consecutive arrays only the second one of which is terminated, for example like so:

const char a[][4] = { "1234", "56" };

int f (void)
{
  return strnlen (a[0], sizeof a);
}

I see that aggregates aren't handled by the folder yet so this is only hypothetical at the moment. But eventually I think there is an expectation to handle them and then the question of whether use cases like the above should be supported will need to be answered.

710

Yes, I think we can. It doesn't seem like there are any cases when minStringLength() could fold away the memory load.

768

Done, thanks.

777

I've simplified/changed the conditional while moving it earlier.

llvm/test/Transforms/InstCombine/strlen-4.ll
4

I have run the script to update the assertions.

llvm/test/Transforms/InstCombine/strnlen-6.ll
53

Yes, there are a couple of typos here, one keeping the other from causing the test to fail. Good catch! There were a few others just like that in a couple of tests.

msebor updated this revision to Diff 419587.Mar 31 2022, 4:29 PM

v2 with changes suggested in review. (I'm still learning the review process and tools so hopefully I didn't do something too stupid.)

nikic added a comment.Apr 1 2022, 8:26 AM

Thanks for the review! Let me upload a new diff with the suggested changes. I'm not sure what the process is for submitting the baseline tests: do I need to open a separate review for those? Likewise for the nonnull attribute for strnlen? I'm also not sure I have commit rights yet so if/when any of this is approved someone might need to commit the changes for me.

For baseline tests, you can just commit them directly. As you don't have commit access yet, I went ahead and committed them for you. But otherwise yes, the usual pattern is to create a patch stack (with "Depends on DNNN") where each patch adds one transform. That makes it easier to review and bisect problems later. (Though there may be some reasonable disagreement on how fine-grained changes should be...)

Finally, the build bot shows unit test failures that I'm not sure I understand yet and could use some guidance with. Are ASan test timeouts expected? Any idea about the BOLT failures?

Unfortunately the pre-merge checks have been pretty flaky lately. If the test times out, you can generally assume it's a false positive. Generally, if it doesn't look obviously related to the area you changed, it's probably a false positive.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
672

I don't think getConstantDataArrayInfo() could return true for such a case, it needs to return the string up to the end of the global. Though now that I look at the implementation, it does support an offset into the global, so if the GEP has a negative index it could move the start of the search before a null byte, and we'd get something less than the bound in that case.

I'm having a hard time convincing myself of the correctness of the code as implemented -- I'd suggest to put the entire dynamic GEP handling behind a !Bound check for the initial patch and revisit this afterwards.

710

Drop the check now?

747

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.

xbolva00 added inline comments.Apr 6 2022, 4:47 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
770

This is an opportunity to extend this opt more

see https://github.com/llvm/llvm-project/issues/48940

msebor added inline comments.Apr 6 2022, 9:30 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
672

Okay, let me break up the patch then and resubmit.

710

Right, will do.

747

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.

770

Let me look into it, thanks for the pointer!

msebor updated this revision to Diff 421009.Apr 6 2022, 2:14 PM

"put the entire dynamic GEP handling behind a !Bound check"

@nikic is this what close to what you're looking for?

nikic added inline comments.Apr 11 2022, 8:15 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
747

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.

msebor added inline comments.Apr 14 2022, 10:20 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
747

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.

Since @nikic requested to break this patch up into a series I'm going to abandon this review and open new ones, one for each of the smaller pieces.

msebor abandoned this revision.Apr 14 2022, 10:23 AM

Abandoning as per previous comment.