[PATCH] D128089: [InstCombine] Fold strncmp of constant arrays and variable size

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1173
+    if (Pos == MinSize ||
+        (StrNCmp && (LStr[Pos] == '\0' && RStr[Pos] == '\0'))) {
+      // One array is a leading part of the other of equal or greater
----------------
courbet wrote:
> nikic wrote:
> > msebor wrote:
> > > nikic wrote:
> > > > 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?
> > > 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.
> > > 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.)
> >> 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 ?
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?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D128089/new/

https://reviews.llvm.org/D128089