[PATCH] D88460: Strlen loop idiom recognition

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

(For future reference, Phabricator has an option  "commandeer" a revision, to take control if the original author of a patch leaves a patch incomplete for whatever reason.  Obviously use sparingly.)



================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1639
+
+  // The loop exit must be conditioned on an icmp with 0.
+  // The icmp operand has to be a load on some SSA reg that increments
----------------
shawnl wrote:
> If the passed pointer has the "nonnull" attribute then this condition need not be present.
The icmp isn't against a pointer; it's against an i8.  Or, it should be; I guess the code is missing a check that it's i8, as opposed to some arbitrary integer type.

For comparison, it's undefined behavior to call strlen with a null pointer.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1691
+  }
+  if (!ResInst)
+    return false;
----------------
I'm not sure how ResInst is connected to the rest of this transform; why are we transforming the operand of some random PHI node?


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1708
+
+  // Remove the loop-exit branch and delete death instructions
+  RecursivelyDeleteTriviallyDeadInstructions(ResInst, TLI);
----------------
"dead instructions", not "death instructions".


================
Comment at: llvm/test/Transforms/LoopIdiom/recognize-strlen.ll:1
+; RUN: opt -loop-idiom < %s -S | FileCheck %s
+target datalayout = "e-m:e-i64:64-n32:64"
----------------
Can you generate the check lines use update_test_checks.py?  It isn't obvious what the whole function looks like as a result of this transform.


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

https://reviews.llvm.org/D88460