[PATCH] D39232: [CodeGen][ExpandMemcmp] Allow memcmp to expand to vector loads (2).

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: include/llvm/Analysis/TargetTransformInfo.h:550-551
 
-  /// \brief Enable inline expansion of memcmp
-  bool enableMemCmpExpansion(unsigned &MaxLoadSize) const;
+  /// \brief If not nullptr, enable inline expansion of memcmp. IsThreeWay is
+  /// false if this is the expansion of memcmp(p1, p2, s) == 0.
+  struct MemCmpExpansionOptions {
----------------
We should avoid using different vocabulary in this API than what is in the expansion code. Instead of 'IsThreeWay' here and other places, we can use 'IsZeroCmp'?


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:2555
+    TTI::MemCmpExpansionOptions Options;
+    if (ST->hasAVX512()) Options.LoadSizes.push_back(64);
+    if (ST->hasAVX()) Options.LoadSizes.push_back(32);
----------------
AVX512 needs to be a 'TODO' in this patch. We're going to need to add more tests (64- and 128-byte), and the DAG is not prepared for those patterns yet.


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:2556-2557
+    if (ST->hasAVX512()) Options.LoadSizes.push_back(64);
+    if (ST->hasAVX()) Options.LoadSizes.push_back(32);
+    if (ST->hasSSE1()) Options.LoadSizes.push_back(16);
+    if (ST->is64Bit()) {
----------------
This isn't correct (or at least it doesn't match what the DAG handles optimally). I've added extra runs to memcmp.ll, so we can see what happens for SSE1/AVX1 vs. SSE2/AVX2.


================
Comment at: test/CodeGen/X86/memcmp.ll:26
 ; X86-NEXT:    subl %ecx, %eax
-; X86-NEXT:    retl
+; X86-NEXT:    ret{{[l|q]}}
 ;
----------------
Apologies for all this noise. I updated the script again at rL316443 to avoid the 'ret' diffs.
I also added extra run lines for SSE1 and AVX1 at rL316446, so you'll need to rebase.


https://reviews.llvm.org/D39232