This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold strnlen calls in equality to zero.
ClosedPublic

Authored by msebor on Apr 14 2022, 1:23 PM.

Details

Summary

This patch in the strnlen series adds folding of calls used in equality expressions with zero.

Depends on D123817.

Diff Detail

Event Timeline

msebor created this revision.Apr 14 2022, 1:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 1:23 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
msebor requested review of this revision.Apr 14 2022, 1:23 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 1:23 PM
msebor edited the summary of this revision. (Show Details)Apr 14 2022, 1:34 PM
xbolva00 added inline comments.Apr 14 2022, 2:25 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
648

maybe you can drop “strlenfirst” here..

xbolva00 added inline comments.Apr 14 2022, 2:29 PM
llvm/test/Transforms/InstCombine/strnlen-5.ll
42

What about “strnlen(…) < 1” ? Do we have such test?

msebor added inline comments.Apr 25 2022, 10:59 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
648

Because the function also handles strnlen besides strlen? I can do that but since other loads in the file are annotated I've changed it to "char0". Let me know if I misunderstood.

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

I've added a couple. I expected isOnlyUsedInZeroEqualityComparison() to return true for such expressions but it only handles straight equality. The case is folded by the subsequent code path but the emitted code is slightly different. It seems it might be worth looking into enhancing the function to handle both < 1 and == 0 with unsigned operands (as well as the converse).

xbolva00 added inline comments.Apr 25 2022, 12:07 PM
llvm/test/Transforms/InstCombine/strnlen-5.ll
42

No need as < 1 is always canonicalized to == 0.

nikic accepted this revision.Apr 27 2022, 7:15 AM

LG -- the current baseline checks are outdated and a bit confusing for that reason, but otherwise this looks right.

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

Looks like this rename did not happen (though I don't really care either way).

This revision is now accepted and ready to land.Apr 27 2022, 7:15 AM
This revision was landed with ongoing or failed builds.Apr 27 2022, 11:06 AM
This revision was automatically updated to reflect the committed changes.
msebor added inline comments.Apr 27 2022, 11:06 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
648

It did, I just haven't posted the updated patch yet. Now that it's been accepted I'll go ahead and commit it with this change.

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

Yes, ultimately it is, but it seems to me that it would be helpful to follow the same code path and make the same folding decisions earlier on, in the libcall handler. I don't have a use case for it yet where it would make a difference (the function isn't widely used) but if one comes up I'll revisit it then,