[PATCH] D28637: [PPC] Inline expansion of memcmp

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: include/llvm/IR/Instruction.h:483
+  /// Return true if only matters that the value is equal or not-equal to zero.
+  bool isOnlyUsedInZeroEqualityComparison() const;
+
----------------
Better to put this in ValueTracking.h, or something like that.


================
Comment at: include/llvm/Target/TargetLowering.h:1016
+  ///
+  /// This function returns the maximum size in bytes to load when
+  /// expanding memcmp. The value is set by the target at the
----------------
The maximum size in bytes isn't really a useful metric; what actually matters for performance/codesize is the number of loads.  (It takes roughly the same number of instructions to memcmp 15 bytes vs. 32 bytes.)


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2122
+
+  LoadSrc1 = Builder.CreateZExtOrTrunc(LoadSrc1, Type::getInt32Ty(Context));
+  LoadSrc2 = Builder.CreateZExtOrTrunc(LoadSrc2, Type::getInt32Ty(Context));
----------------
CreateZExt.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2233
+    Builder.Insert(NewBr);
+    return;
+  }
----------------
Hmm... in isOnlyUsedInZeroEqualityComparison mode, it probably makes sense to perform more loads per block, to reduce the number of branches.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2269
+  Value *Subtract = Builder.CreateSub(And1, And2);
+  Value *Res = Builder.CreateSExtOrTrunc(Subtract, Builder.getInt32Ty());
+
----------------
Would `bswap(Src1) > bswap(Src2) ? 1 : -1` be shorter?


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:2639
+  LibFunc Func;
+  if (TLInfo->getLibFunc(CI->getCalledFunction()->getName(), Func) &&
+      Func == LibFunc_memcmp) {
----------------
Better to use the overload of getLibFunc that takes a `Function&`.


https://reviews.llvm.org/D28637