[AggressiveInstCombine][NFC] Fix typo

[kitaisreal]

Summary

Fix typo in expandStrcmp method.

[goldstein.w.n]

llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp

1026

I don't really understand this, can you add a comment explaining this.

1037

Can you join the logic earlier? Imo Should be a forward declare value for the cmp result but the BR and BB propagation should be common codes.

[nikic]

The general alternative here is to not make AggressiveInstCombine produce better code, but rather teach something else to optimize the pattern it does produce, with the benefit that it can also help other code.

This seems like foldOpIntoPhi() for icmps should nearly handle -- I expect the problem is that we lose the fact that the icmp of the phi is false for one incoming value, because that fact is only known on the edge, while we perform simplification at the terminator of the predecessor. Adding a special case for that based on isImpliedCond should probably be sufficient to simplify this pattern.

[kitaisreal]

In D156556#4543191, @nikic wrote:

The general alternative here is to not make AggressiveInstCombine produce better code, but rather teach something else to optimize the pattern it does produce, with the benefit that it can also help other code.

This seems like foldOpIntoPhi() for icmps should nearly handle -- I expect the problem is that we lose the fact that the icmp of the phi is false for one incoming value, because that fact is only known on the edge, while we perform simplification at the terminator of the predecessor. Adding a special case for that based on isImpliedCond should probably be sufficient to simplify this pattern.

Thanks. Improved foldOpIntoPhi() in this revision D156620.

[kitaisreal]

Transformed this to fix typo after https://reviews.llvm.org/D156620 was landed.

[kitaisreal]

Uploaded correct patch.

[goldstein.w.n]

Can you mark the title as NFC?

[kitaisreal]

In D156556#4554436, @goldstein.w.n wrote:

Can you mark the title as NFC?

It seems that title of revision is "[AggressiveInstCombine][NFC] Fix typo". Do you mean to put NFC at the beginning ?

[goldstein.w.n]

In D156556#4555036, @kitaisreal wrote:

In D156556#4554436, @goldstein.w.n wrote:

Can you mark the title as NFC?

It seems that title of revision is "[AggressiveInstCombine][NFC] Fix typo". Do you mean to put NFC at the beginning ?

No its good. I just can't read.
LGTM.

llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp

if (!HasStr1 && HasStr2) {
     ConstantStr = Str1;
   } else {
     return false;
   }
 
   size_t ConstantStrSize = ConstantStr.size();
 
   // Trivial cases are optimized during inst combine
-  if (ConstantStrSize == 0) {
+  if (ConstantStrSize == 0 || ConstantStrSize > 2)
     return false;
-  }
-
-  if (ConstantStrSize > 2) {
-    return false;
-  }
 
   // Check if strcmp result is only used in a comparison with zero
   if (!isOnlyUsedInZeroComparison(CI))
     return false;
 
   // For strcmp(P, "x") do the following transformation:
   //
   // (before)
   // dst = strcmp(P, "x")
   //
   // (after)
   // v0 = P[0] - 'x'
   // [if v0 == 0]
   //   v1 = P[1]
   // dst = phi(v0, v1)
   //
   // For strcmp(P, "xy") do the following transformation:
   //
   // (before)
-  // dst = strcmp(P, "x")
+  // dst = strcmp(P, "xy")
   //
   // (after)
   // v0 = P[0] - 'x'
   // [if v0 == 0]
   //   v1 = P[1] - 'y'
   //   [if v1 == 0]
   //     v2 = P[2]
   // dst = phi(v0, v1, v2)
Value *StrCharacterValue = B.CreateZExt(
                      B.CreateConstInBoundsGEP1_64(B.getInt8Ty(), NonConstantP,
                                                   CharacterIndexToCheck)),
         RetType);
     Value *ConstantStrCharacterValue = ConstantInt::get(
         RetType,
         static_cast<unsigned char>(ConstantStr[CharacterIndexToCheck]));
     Value *CharacterSub =
         B.CreateNSWSub(StrCharacterValue, ConstantStrCharacterValue);
-    Value *IsCharacterSubZero =
+    Value *CharacterSubIsZero =
         B.CreateICmpEQ(CharacterSub, ConstantInt::get(RetType, 0));
-    BasicBlock *IsCharacterSubZeroBB =
+    BasicBlock *CharacterSubIsZeroBB =
         BasicBlock::Create(B.getContext(), "strcmp_expand_sub_is_zero",
                            InitialBB->getParent(), JoinBlock);
-    B.CreateCondBr(IsCharacterSubZero, IsCharacterSubZeroBB, JoinBlock);
+    B.CreateCondBr(CharacterSubIsZero, CharacterSubIsZeroBB, JoinBlock);
 
     ResultPHI->addIncoming(CharacterSub, B.GetInsertBlock());
     DTUpdates.emplace_back(DominatorTree::Insert, B.GetInsertBlock(),
-                           IsCharacterSubZeroBB);
+                           CharacterSubIsZeroBB);
 
-    B.SetInsertPoint(IsCharacterSubZeroBB);
-    DTUpdates.emplace_back(DominatorTree::Insert, IsCharacterSubZeroBB,
+    B.SetInsertPoint(CharacterSubIsZeroBB);
+    DTUpdates.emplace_back(DominatorTree::Insert, CharacterSubIsZeroBB,
                            JoinBlock);
   }
 
   Value *StrLastCharacterValue = B.CreateZExt(
       B.CreateLoad(B.getInt8Ty(),
                    B.CreateConstInBoundsGEP1_64(B.getInt8Ty(), NonConstantP,
                                                 CharacterIndexToCheck)),
       RetType);
   ResultPHI->addIncoming(StrLastCharacterValue, B.GetInsertBlock());
   B.CreateBr(JoinBlock);
 
   DTU.applyUpdates(DTUpdates);
 
   CI->replaceAllUsesWith(ResultPHI);
   CI->eraseFromParent();
 
   MadeCFGChange = true;
 
   return true;
 }
 
 static bool foldLibraryCalls(Instruction &I, TargetTransformInfo &TTI,
                              TargetLibraryInfo &TLI, DominatorTree &DT,
                              AssumptionCache &AC, bool &MadeCFGChange) {
   CallInst *CI = dyn_cast<CallInst>(&I);
   if (!CI)
     return false;
 
   LibFunc Func;
   Module *M = I.getModule();
   if (!TLI.getLibFunc(*CI, Func) || !isLibFuncEmittable(M, &TLI, Func))
     return false;
 
   switch (Func) {
   case LibFunc_sqrt: