This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold memrchr calls with constant size of zero or one.
ClosedPublic

Authored by msebor on Apr 12 2022, 1:30 PM.

Details

Summary

This change implements folding of memrchr calls with constant sizes of zero and one, analogously to memchr.

Diff Detail

Event Timeline

msebor created this revision.Apr 12 2022, 1:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 1:30 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
msebor requested review of this revision.Apr 12 2022, 1:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2022, 1:30 PM
nikic added a comment.Apr 12 2022, 2:19 PM

Could you please upload the patch with context (pass -U99999 to git show/git diff)?

xbolva00 added inline comments.Apr 12 2022, 2:39 PM
llvm/test/Transforms/InstCombine/memrchr-3.ll
91

nit: please also set some names to parameters to avoid %0, %1, etc..

msebor updated this revision to Diff 422355.Apr 12 2022, 4:06 PM

Add context.

nikic added inline comments.Apr 13 2022, 2:35 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
873

I'm not entirely sure this is correct. This is definitely incorrect for memchr() and I submitted D123665 to fix that. memrchr() is a GNU extension, so I'm not sure whether the same rules apply there. I guess one could argue that memrchr() should require a fully dereferenceable object, as otherwise the base pointer will already point outside the object.

nikic added inline comments.Apr 13 2022, 2:54 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
873

As a data point, sanitizer interception for these functions is implemented as follows: https://github.com/llvm/llvm-project/blob/ee44896cf42d3360f7e0229c360258c92262a47b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc#L984-L1021 So while the interceptor does not assume full dereferenceability for memchr(), it does assume so for memrchr(). So your current implementation would be consistent with sanitizers.

xbolva00 added inline comments.Apr 13 2022, 3:39 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
873

Any sanitizer developer here?

It would be great to know why there is a such differenr behaviour.

xbolva00 added inline comments.Apr 13 2022, 3:44 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
873

“ I guess one could argue that memrchr() should require a fully dereferenceable object, as otherwise the base pointer will already point outside the object”

But yeah this makes sense

msebor added inline comments.Apr 13 2022, 8:53 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
873

Good catch on the memchr annotation! For memrchr, as you note, the whole range [base, base + n) must be dereferenceable even if it's not necessarily always accessed.

nikic accepted this revision.Apr 13 2022, 12:25 PM

LGTM

This revision is now accepted and ready to land.Apr 13 2022, 12:25 PM
msebor added inline comments.Apr 13 2022, 12:52 PM
llvm/test/Transforms/InstCombine/memrchr-3.ll
91

Sure, let me do that separately: rGb8bac957d18b.

This revision was landed with ongoing or failed builds.Apr 26 2022, 1:03 PM
This revision was automatically updated to reflect the committed changes.