This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fold strncmp of constant arrays and variable size
ClosedPublic

Authored by msebor on Jun 17 2022, 12:05 PM.

Details

Summary

This change extends the solution accepted in D127766 to strncmp: it simplifies strncmp(A, B, N) calls with constant A and B and variable N to the equivalent of

N <= Pos ? 0 : (A < B ? -1 : B < A ? +1 : 0)

where Pos is the offset of either the first mismatch between A and B or the terminating null character if both A and B are equal strings.

I couldn't find a good way to preserve the form of the loop in optimizeMemCmpVarSize so this change reverts it to the form originally proposed in D127766.

Diff Detail

Event Timeline

msebor created this revision.Jun 17 2022, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 12:05 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
msebor requested review of this revision.Jun 17 2022, 12:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2022, 12:05 PM

Tested by running make check-all on x86_64-linux.

Can you please upload the patch with full context?

msebor updated this revision to Diff 438056.Jun 17 2022, 3:42 PM

Add more context.

nikic added inline comments.Jun 21 2022, 1:12 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1173

Not really sure why the change in code structure is needed. As far as I can tell, the only thing that's actually changing here is that a

if (StrNCmp && (LStr[Pos] == '\0' && RStr[Pos] == '\0'))
  break;

condition gets added to the top of the loop body? Or am I misunderstanding something here?

1181

Can just return Zero here?

llvm/test/Transforms/InstCombine/strncmp-5.ll
94

Verify

msebor updated this revision to Diff 438721.Jun 21 2022, 8:39 AM
msebor added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1173

The result is zero in this case, not +/-1 as the current algorithm computes. Either way it seems like six of one vs half a dozen of the other.

nikic added inline comments.Jun 21 2022, 9:23 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1173

The result is zero in this case, not +/-1 as the current algorithm computes.

I don't really follow -- I just tested this, and it at least passes the tests.

Either way it seems like six of one vs half a dozen of the other.

Agree that in terms of final outcome both are pretty much the same and I wouldn't care either way. However, this makes the patch much simpler, because it becomes a 2-line logic change. (For reference, it looks like this is the original structure from D127766, which was then changed based on a comment from @courbet, and this restores the original again.)

I prefer this final version. Let me know if it's okay.

nikic added a comment.Jun 22 2022, 3:56 AM

Fine by me, but I'll leave the approval to @courbet.

courbet added inline comments.Jun 23 2022, 6:30 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1173

The result is zero in this case, not +/-1 as the current algorithm computes.

I don't really follow -- I just tested this, and it at least passes the tests.

If we can change the code to compute something different and it does not break the tests, can you add a test case that shows the issue ?

msebor added inline comments.Jun 23 2022, 7:47 AM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1173

I'm not sure I understand the concern but my impression is that we're debating the difference between the following two equivalent forms of the loop (on trunk):

for (...) {
  if (x) {
    do-stuff;
    break;
  }
}

and (in the patch):

for (...) {
  if (x)
    break;
}
do-stuff;

The "+/-1" part of my response was to the question that

...the only thing that's actually changing here is that the if (StrNCmp && (LStr[Pos] == '\0' && RStr[Pos] == '\0')) break;condition gets added to the top of the loop body?

@courbet: Please let me know if you have any further questions or suggestions, otherwise I'll go ahead and commit the final revision by the end of the week. Thanks!

courbet accepted this revision.Jun 28 2022, 7:50 AM

I'm not sure I understand the concern but my impression is that we're debating the difference between the following two equivalent forms of the loop

OK, I was afraid you were implying that they were not equivalent.

SGTM then.

This revision is now accepted and ready to land.Jun 28 2022, 7:50 AM
This revision was landed with ongoing or failed builds.Jun 28 2022, 3:00 PM
This revision was automatically updated to reflect the committed changes.
msebor marked an inline comment as done.Jun 28 2022, 3:04 PM