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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

(Review interrupted, a few useful comments, more to come)



================
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");
----------------
lebedev.ri wrote:
> reames wrote:
> > Just mod 8.  LLVM does not support bytes of other size.
> Is this sufficiently-obscure, or shall i inline the variable, too?
Looks fine.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:2127
+      LoadA->getPointerAddressSpace() != 0 || !LoadA->isSimple() ||
+      !LoadB->isSimple()) {
+    StringLiteral L("Unsupported loads in idiom - only support identical, "
----------------
lebedev.ri wrote:
> reames wrote:
> > Actually, you do handle volatile.  Move this closer to the def for readability would you?
> The important difference is - if i bailout at the very beginning, no further analysis will be done.
> But if i bailout here, then if this fires then one who sees this diagnostic may know that
> the issue here isn't the volatile loads themselves, but rather the missing codegen.
> As in, all other legality checks have passed here.
> 
> With that info, the comment still stands?
Really not sure what that gives the user?  If the loop contains a volatile or ordered load, we're done.  The transform isn't going to run, no matter what other legality predicates are met.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1924
+  if (!match(LoopHeaderBB->getTerminator(),
+             m_Br(m_CombineAnd(m_ICmp(BCmpPred, m_Value(), m_Value()),
+                               m_Value(BCmpValue)),
----------------
Using m_CombineAnd like this is correct, but I found quite confusing.  You might want to consider pulling out a helper function and using early return matching instead.

e.g.
struct MatchedBCmpCompairsonState {...};
bool matchBCmpComparison(Instruction *Term, MatchState&)



================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:2020
+    for (const PHINode &PHI : ExitBB->phis()) {
+      for (const BasicBlock *LoopBB :
+           make_filter_range(PHI.blocks(), [this](BasicBlock *PredecessorBB) {
----------------
If the loop has unique exits, then each phi must have a single predecessor which must be in the loop.  LoopSimplify creates unique exits, so I'd just bail on the non-unique case.


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:2036
+  }
+  // Similarly, the loop should not have any other observable side-effects
+  // other than the final comparison result.
----------------
Given the base of each object and the length, you can form a MemoryLocation for the entire portion being read from both arrays.  Given that, a simple alias query can check for modification or ordering.  

MemoryLocation LocA(Src1, Size);
for (Instruction &I : instructions(L))
  if (I aliases LocA) return false;


Note that if you length is non-constant, this ends up being quite conservative, but so does your current code.


See also: mayLoopAccessLocation


================
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
 
----------------
lebedev.ri wrote:
> reames wrote:
> > Unrelated test change.  Remove please.
> How is this unrelated? This is one of the dedicated test files for this diff,
> and that change is showing that the patch does not anger any of these verifiers.
Presumably it didn't anger any of the verifiiers before, so the change can be checked in and doesn't need to be part of this diff.


================
Comment at: test/Transforms/LoopIdiom/bcmp-debugify-remarks.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
-; RUN: opt -debugify -loop-idiom < %s -S 2>&1 | FileCheck %s
+; RUN: opt -debugify -loop-idiom -pass-remarks=loop-idiom -pass-remarks-analysis=loop-idiom -verify -verify-each -verify-dom-info -verify-loop-info < %s -S 2>&1 | FileCheck %s
 
----------------
Same here.


================
Comment at: test/Transforms/LoopIdiom/bcmp-widening.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
 
----------------
Same here.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61144