This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyLibCalls] Remove invalid code comment in optimizeStringLength. NFC
ClosedPublic

Authored by bjope on Oct 5 2022, 5:43 AM.

Details

Summary

The limitation in LibCallSimplifier::optimizeStringLength to only
optimize when the string is an i8 array was changed already in
commit 50ec0b5dceff1f3fe93 back in 2017.

We still only simplify when 's 'points at an array of 'CharSize', so
the comment is still valid in the sense that we do not support
arbitrary array types.

Diff Detail

Event Timeline

bjope created this revision.Oct 5 2022, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 5:43 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
bjope requested review of this revision.Oct 5 2022, 5:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 5 2022, 5:43 AM
nikic added a subscriber: nikic.Oct 5 2022, 6:06 AM

I believe this comment is about supporting arbitrary types, not just extension to wchar.

bjope added a comment.Oct 5 2022, 7:06 AM

I believe this comment is about supporting arbitrary types, not just extension to wchar.

Alright. I see! So my confusion probably was that it says "i8" rather than "CharSize".

Before commit 50ec0b5dceff1f3fe93 the isGEPBasedOnPointerToString helper did limit this to i8 (but then we did not support CharSize!=8 here), so back then it was really limited to just i8. But nowadays I think the array need to match with "CharSize".
Still, as you say, we aren't supporting arbitrary types for the array elements (they need to match with "CharSize")!

bjope updated this revision to Diff 465387.Oct 5 2022, 7:13 AM

Now just adjusting the code comment.

Description:

[SimplifyLibCalls] Adjust code comment in optimizeStringLength. NFC

The limitation in LibCallSimplifier::optimizeStringLength to only
optimize when the string is an i8 array was changed already in
commit 50ec0b5dceff1f3fe93 back in 2017.

We still only simplify when 's 'points at an array of 'CharSize', so
the comment is still valid in the sense that we do not support
arbitrary array types.
bjope edited the summary of this revision. (Show Details)Oct 5 2022, 7:13 AM
nikic accepted this revision.Oct 5 2022, 7:39 AM

LG

This revision is now accepted and ready to land.Oct 5 2022, 7:39 AM