Address the review comments

igogo-x86 · igogo-x86 · commit 76866771222a · 2023-10-27T10:36:05.000Z
diff --git a/llvm/lib/CodeGen/ExpandMemCmp.cpp b/llvm/lib/CodeGen/ExpandMemCmp.cpp
@@ -117,7 +117,7 @@ class MemCmpExpansion {
     Value *Lhs = nullptr;
     Value *Rhs = nullptr;
   };
-  LoadPair getLoadPair(Type *LoadSizeType, bool NeedsBSwap, Type *BSwapSizeType,
+  LoadPair getLoadPair(Type *LoadSizeType, Type *BSwapSizeType,
                        Type *CmpSizeType, unsigned OffsetBytes);
 
   static LoadEntryVector
@@ -128,6 +128,11 @@ class MemCmpExpansion {
                                  unsigned MaxNumLoads,
                                  unsigned &NumLoadsNonOneByte);
 
+  static void optimiseLoadSequence(
+      LoadEntryVector &LoadSequence,
+      const TargetTransformInfo::MemCmpExpansionOptions &Options,
+      bool IsUsedForZeroCmp);
+
 public:
   MemCmpExpansion(CallInst *CI, uint64_t Size,
                   const TargetTransformInfo::MemCmpExpansionOptions &Options,
@@ -210,6 +215,37 @@ MemCmpExpansion::computeOverlappingLoadSequence(uint64_t Size,
   return LoadSequence;
 }
 
+void MemCmpExpansion::optimiseLoadSequence(
+    LoadEntryVector &LoadSequence,
+    const TargetTransformInfo::MemCmpExpansionOptions &Options,
+    bool IsUsedForZeroCmp) {
+  // This part of code attempts to optimize the LoadSequence by merging allowed
+  // subsequences into single loads of allowed sizes from
+  // `MemCmpExpansionOptions::AllowedTailExpansions`. If it is for zero
+  // comparison or if no allowed tail expansions are specified, we exit early.
+  if (IsUsedForZeroCmp || Options.AllowedTailExpansions.empty())
+    return;
+
+  while (LoadSequence.size() >= 2) {
+    auto Last = LoadSequence[LoadSequence.size() - 1];
+    auto PreLast = LoadSequence[LoadSequence.size() - 2];
+
+    // Exit the loop if the two sequences are not contiguous
+    if (PreLast.Offset + PreLast.LoadSize != Last.Offset)
+      break;
+
+    auto LoadSize = Last.LoadSize + PreLast.LoadSize;
+    if (find(Options.AllowedTailExpansions, LoadSize) ==
+        Options.AllowedTailExpansions.end())
+      break;
+
+    // Remove the last two sequences and replace with the combined sequence
+    LoadSequence.pop_back();
+    LoadSequence.pop_back();
+    LoadSequence.emplace_back(PreLast.Offset, LoadSize);
+  }
+}
+
 // Initialize the basic block structure required for expansion of memcmp call
 // with given maximum load size and memcmp size parameter.
 // This structure includes:
@@ -255,31 +291,7 @@ MemCmpExpansion::MemCmpExpansion(
     }
   }
   assert(LoadSequence.size() <= Options.MaxNumLoads && "broken invariant");
-  // This part of code attempts to optimize the LoadSequence by merging allowed
-  // subsequences into single loads of allowed sizes from
-  // `AllowedTailExpansions`. If it is for zero comparison or if no allowed tail
-  // expansions are specified, we exit early.
-  if (IsUsedForZeroCmp || !Options.AllowedTailExpansions.size())
-    return;
-
-  while (LoadSequence.size() >= 2) {
-    auto Last = LoadSequence[LoadSequence.size() - 1];
-    auto PreLast = LoadSequence[LoadSequence.size() - 2];
-
-    // Exit the loop if the two sequences are not contiguous
-    if (PreLast.Offset + PreLast.LoadSize != Last.Offset)
-      break;
-
-    auto LoadSize = Last.LoadSize + PreLast.LoadSize;
-    if (find(Options.AllowedTailExpansions, LoadSize) ==
-        Options.AllowedTailExpansions.end())
-      break;
-
-    // Remove the last two sequences and replace with the combined sequence
-    LoadSequence.pop_back();
-    LoadSequence.pop_back();
-    LoadSequence.emplace_back(PreLast.Offset, LoadSize);
-  }
+  optimiseLoadSequence(LoadSequence, Options, IsUsedForZeroCmp);
 }
 
 unsigned MemCmpExpansion::getNumBlocks() {
@@ -303,7 +315,6 @@ void MemCmpExpansion::createResultBlock() {
 }
 
 MemCmpExpansion::LoadPair MemCmpExpansion::getLoadPair(Type *LoadSizeType,
-                                                       bool NeedsBSwap,
                                                        Type *BSwapSizeType,
                                                        Type *CmpSizeType,
                                                        unsigned OffsetBytes) {
@@ -334,13 +345,13 @@ MemCmpExpansion::LoadPair MemCmpExpansion::getLoadPair(Type *LoadSizeType,
     Rhs = Builder.CreateAlignedLoad(LoadSizeType, RhsSource, RhsAlign);
 
   // Zero extend if Byte Swap intrinsic has different type
-  if (NeedsBSwap && LoadSizeType != BSwapSizeType) {
+  if (BSwapSizeType && LoadSizeType != BSwapSizeType) {
     Lhs = Builder.CreateZExt(Lhs, BSwapSizeType);
     Rhs = Builder.CreateZExt(Rhs, BSwapSizeType);
   }
 
   // Swap bytes if required.
-  if (NeedsBSwap) {
+  if (BSwapSizeType) {
     Function *Bswap = Intrinsic::getDeclaration(
         CI->getModule(), Intrinsic::bswap, BSwapSizeType);
     Lhs = Builder.CreateCall(Bswap, Lhs);
@@ -364,8 +375,8 @@ void MemCmpExpansion::emitLoadCompareByteBlock(unsigned BlockIndex,
   BasicBlock *BB = LoadCmpBlocks[BlockIndex];
   Builder.SetInsertPoint(BB);
   const LoadPair Loads =
-      getLoadPair(Type::getInt8Ty(CI->getContext()), /*NeedsBSwap=*/false,
-                  nullptr, Type::getInt32Ty(CI->getContext()), OffsetBytes);
+      getLoadPair(Type::getInt8Ty(CI->getContext()), nullptr,
+                  Type::getInt32Ty(CI->getContext()), OffsetBytes);
   Value *Diff = Builder.CreateSub(Loads.Lhs, Loads.Rhs);
 
   PhiRes->addIncoming(Diff, BB);
@@ -421,8 +432,8 @@ Value *MemCmpExpansion::getCompareLoadPairs(unsigned BlockIndex,
   for (unsigned i = 0; i < NumLoads; ++i, ++LoadIndex) {
     const LoadEntry &CurLoadEntry = LoadSequence[LoadIndex];
     const LoadPair Loads = getLoadPair(
-        IntegerType::get(CI->getContext(), CurLoadEntry.LoadSize * 8),
-        /*NeedsBSwap=*/false, nullptr, MaxLoadType, CurLoadEntry.Offset);
+        IntegerType::get(CI->getContext(), CurLoadEntry.LoadSize * 8), nullptr,
+        MaxLoadType, CurLoadEntry.Offset);
 
     if (NumLoads != 1) {
       // If we have multiple loads per block, we need to generate a composite
@@ -508,18 +519,20 @@ void MemCmpExpansion::emitLoadCompareBlock(unsigned BlockIndex) {
 
   Type *LoadSizeType =
       IntegerType::get(CI->getContext(), CurLoadEntry.LoadSize * 8);
-  Type *BSwapSizeType = IntegerType::get(
-      CI->getContext(), PowerOf2Ceil(CurLoadEntry.LoadSize * 8));
+  Type *BSwapSizeType =
+      DL.isLittleEndian()
+          ? IntegerType::get(CI->getContext(),
+                             PowerOf2Ceil(CurLoadEntry.LoadSize * 8))
+          : nullptr;
   Type *MaxLoadType = IntegerType::get(
       CI->getContext(),
       std::max(MaxLoadSize, (unsigned)PowerOf2Ceil(CurLoadEntry.LoadSize)) * 8);
   assert(CurLoadEntry.LoadSize <= MaxLoadSize && "Unexpected load type");
 
   Builder.SetInsertPoint(LoadCmpBlocks[BlockIndex]);
 
-  const LoadPair Loads =
-      getLoadPair(LoadSizeType, /*NeedsBSwap=*/DL.isLittleEndian(),
-                  BSwapSizeType, MaxLoadType, CurLoadEntry.Offset);
+  const LoadPair Loads = getLoadPair(LoadSizeType, BSwapSizeType, MaxLoadType,
+                                     CurLoadEntry.Offset);
 
   // Add the loaded values to the phi nodes for calculating memcmp result only
   // if result is not used in a zero equality.
@@ -624,27 +637,25 @@ Value *MemCmpExpansion::getMemCmpEqZeroOneBlock() {
 /// A memcmp expansion that only has one block of load and compare can bypass
 /// the compare, branch, and phi IR that is required in the general case.
 Value *MemCmpExpansion::getMemCmpOneBlock() {
+  bool NeedsBSwap = DL.isLittleEndian() && Size != 1;
   Type *LoadSizeType = IntegerType::get(CI->getContext(), Size * 8);
   Type *BSwapSizeType =
-      IntegerType::get(CI->getContext(), PowerOf2Ceil(Size * 8));
+      NeedsBSwap ? IntegerType::get(CI->getContext(), PowerOf2Ceil(Size * 8))
+                 : nullptr;
   Type *MaxLoadType =
       IntegerType::get(CI->getContext(),
                        std::max(MaxLoadSize, (unsigned)PowerOf2Ceil(Size)) * 8);
 
-  bool NeedsBSwap = DL.isLittleEndian() && Size != 1;
-
   // The i8 and i16 cases don't need compares. We zext the loaded values and
   // subtract them to get the suitable negative, zero, or positive i32 result.
   if (Size < 4) {
-    const LoadPair Loads = getLoadPair(LoadSizeType, NeedsBSwap, BSwapSizeType,
-                                       Builder.getInt32Ty(),
-                                       /*Offset*/ 0);
+    const LoadPair Loads = getLoadPair(LoadSizeType, BSwapSizeType,
+                                       Builder.getInt32Ty(), /*Offset*/ 0);
     return Builder.CreateSub(Loads.Lhs, Loads.Rhs);
   }
 
-  const LoadPair Loads =
-      getLoadPair(LoadSizeType, NeedsBSwap, BSwapSizeType, MaxLoadType,
-                  /*Offset*/ 0);
+  const LoadPair Loads = getLoadPair(LoadSizeType, BSwapSizeType, MaxLoadType,
+                                     /*Offset*/ 0);
   // The result of memcmp is negative, zero, or positive, so produce that by
   // subtracting 2 extended compare bits: sub (ugt, ult).
   // If a target prefers to use selects to get -1/0/1, they should be able
