[PATCH] D75840: [ExpandMemCmp] Improve non-equality comparisons with zero.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/CodeGen/ExpandMemCmp.cpp:370-374
+  case CmpInst::ICMP_SLT:
+  case CmpInst::ICMP_SGT:
+  case CmpInst::ICMP_SLE:
+  case CmpInst::ICMP_SGE:
+    return getCompareLoadPairsForZeroRelational(BlockIndex, LoadIndex);
----------------
courbet wrote:
> spatel wrote:
> > This is not correct. memcmp() compares as unsigned bytes:
> > http://www.cplusplus.com/reference/cstring/memcmp/
> > 
> > This program will show a failure compiled with -O1 vs -O0 (do we have some kind of correctness test like this in the test-suite?):
> > 
> > 
> > ```
> > #include <string.h>
> > #include <stdio.h>
> > 
> > void wrapper(char *c, char *d) {
> >   if (memcmp(c, d, 4) > 0)
> >     printf(">0\n");
> >   else
> >     printf("<=0\n");
> > }
> > 
> > int main() {
> >   int A = 0x00000000;
> >   int B = 0x00000080;
> >   wrapper(&A, &B);
> >   return 0;
> > }
> > 
> > ```
> Thank for the catch. I've added more tests in `4edd050c7e97` to make the issue more obvious to the eye than the `setl`/`setb` issue.
> 
> I though the test-suite benchmark was validating the result, but it's only benchmarking and throwing the result away. I'll fix the benchmarks.
Great - thanks!
If we want another option that's easier to spot diffs in than x86 asm - add to the IR tests in "llvm/test/Transforms/ExpandMemCmp/".


================
Comment at: llvm/lib/CodeGen/ExpandMemCmp.cpp:680-684
+// If the memcmp is only used for a given comparison against zero,
+// return that predicate.
+static CmpInst::Predicate isOnlyUsedInZeroComparison(const Instruction *CxtI) {
+  CmpInst::Predicate CommonPred = CmpInst::BAD_ICMP_PREDICATE;
+  for (const User *U : CxtI->users()) {
----------------
courbet wrote:
> spatel wrote:
> > Is there a need to check for multiple users that are identical? Ie, doesn't CSE make this unlikely?
> It might be unlikely, but it's not guaranteed.  I'm not sure how I would handle diverging users, did yo have anything specific in mind ?
We could just give up if the memcmp() call !hasOneUse(). It simplifies the code, but it might miss some strange case...but if there's no test coverage/evidence that the multi-user case exists, it should be fine?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75840