[PATCH] D61144: [LoopIdiomRecognize] BCmp loop idiom recognition

[Philip Reames via Phabricator via llvm-commits]

reames requested changes to this revision.
reames added a comment.
This revision now requires changes to proceed.

Initial set of changes to reduce patch complexity.  Note that I've skipped most of the interesting logic and will not review it until unrelated pieces have been split apart.



================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:147
+  PMAbstraction &LoopDeleter;
+  OptimizationRemarkEmitter &ORE;
   bool ApplyCodeSizeHeuristics;
----------------
The addition of ORE appears to be a separable change which causes test diffs of it's own.  Please split and review separately.  


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:376
 
-  LLVM_DEBUG(dbgs() << "loop-idiom Scanning: F["
+  LLVM_DEBUG(dbgs() << DEBUG_TYPE " Scanning: F["
                     << CurLoop->getHeader()->getParent()->getName()
----------------
Unrelated changes, please split and land.  (Applies for almost all LLVM_DEBUG usage.)


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1853
+#undef DEBUG_TYPE
+#define DEBUG_TYPE "loop-idiom-bcmp"
+
----------------
Nope, remove.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1976
+  LoadA = cast<LoadInst>(LoadA_);              // these cast with
+  LoadB = cast<LoadInst>(LoadB_);              // m_Value() matcher?
+
----------------
You don't appear to be bailing out for atomic or volatile loads.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1989
+  // that we are dealing with a multiple of a byte here.
+  if (BCmpTyBits % ByteTyBits != 0) {
+    LLVM_DEBUG(dbgs() << "Value size is not a multiple of byte.\n");
----------------
Just mod 8.  LLVM does not support bytes of other size.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:2127
+      LoadA->getPointerAddressSpace() != 0 || !LoadA->isSimple() ||
+      !LoadB->isSimple()) {
+    StringLiteral L("Unsupported loads in idiom - only support identical, "
----------------
Actually, you do handle volatile.  Move this closer to the def for readability would you?


================
Comment at: test/Transforms/LoopIdiom/bcmp-basic.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -loop-idiom < %s -S | FileCheck %s
+; RUN: opt -loop-idiom -verify -verify-each -verify-dom-info -verify-loop-info < %s -S | FileCheck %s
 
----------------
Unrelated test change.  Remove please.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61144