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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


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


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:2127
+      LoadA->getPointerAddressSpace() != 0 || !LoadA->isSimple() ||
+      !LoadB->isSimple()) {
+    StringLiteral L("Unsupported loads in idiom - only support identical, "
----------------
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?


================
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
 
----------------
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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61144