[PATCH] D127766: [InstCombine] Fold memcmp 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:1163-1168
+      // One array is a leading part of the other of equal or greater
+      // size.
+      // Fold the result to zero.  Size is assumed to be in bounds, since
+      // otherwise the call would be undefined.
+      // Setting Pos to MAX ensures that the N <= Pos test inserted below
+      // is eventually folded away.
----------------
courbet wrote:
> This feels a bit convoluted. Why not something like:
> 
> ```
> Value *Zero = ConstantInt::get(CI->getType(), 0);
> 
> uint64_t MinSize = std::min(LStr.size(), RStr.size());
> for (uint64_t Pos = 0; Pos < MinSize; ++Pos) {
>   if (LStr[Pos] != RStr[Pos]) {
>     Value *MaxSize = ConstantInt::get(Size->getType(), Pos);
>     Value *Cmp = B.CreateICmp(ICmpInst::ICMP_ULE, Size, MaxSize);
>     int IRes = LStr[Pos] < RStr[Pos] ? -1 : 1;
>     Value *Res = ConstantInt::get(CI->getType(), IRes);
>     return B.CreateSelect(Cmp, Zero, Res);
>   }
> }
> // One array is a leading part of the other of equal or greater size.   
> // Fold the result to zero.  Size is assumed to be in bounds, since  
> // otherwise the call would be undefined. 
> return  Zero;
> ``` 
Sure, I can do that, thanks for the suggestion.  (I have a follow-up patch that extends the handling to `strncmp` so the code won't look exactly the same but should stay close enough.)


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:1276
   ConstantInt *LenC = dyn_cast<ConstantInt>(Size);
   if (!LenC)
+    // Hande variable Size.
----------------
nikic wrote:
> courbet wrote:
> > [nit] It feels weird to have `if(!constant) { /*handle general case*/ } /*handle constant case*/ `. Can you rewrite it as  `if(constant) { /*handle constant case*/ } /*handle general case*/ ` ?
> We can probably also always check the variable case first (because it also works fine for the all-constant case, right? The extra condition will fold afterwards) and drop the       getConstantStringInfo() handling from optimizeMemCmpConstantSize().
Yes, that sounds good to me.


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

https://reviews.llvm.org/D127766