[PATCH] D107353: [LoopIdiom] let the pass deal with runtime memset size

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:950
+    // Check if the pointer stride matches the memset size.
+    // To be conservative, the pass would not promote pointers that isn't in
+    // address space zero. Also, the pass only handle memset length and stride
----------------
isn't -> aren't


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:951
+    // To be conservative, the pass would not promote pointers that isn't in
+    // address space zero. Also, the pass only handle memset length and stride
+    // that are invariant for the top level loop.
----------------
handle -> handles


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:954
+    // If the original StrideSCEV and MemsetSizeSCEV does not match, the pass
+    // will fold expressions that is covered by the loop guard at loop entry.
+    // The pass will compare again after the folding and proceed if equal.
----------------
Is this part of the comment talking about what you plan to do in the future? If so please either move it to the TODO section on line 977, or put a TODO marker here.


================
Comment at: llvm/test/Transforms/LoopIdiom/memset-runtime-debug.ll:43
+}
+; void test(int n, int m, int *ar)
+; {
----------------
`int *ar` is actually the first parameter in the IR below.


================
Comment at: llvm/test/Transforms/LoopIdiom/memset-runtime-debug.ll:114
+
+define void @NonAffinePointer(i32* %0, i32 %1, i32 %2) {
+; CHECK: Pointer is not affine, abort
----------------
Would be useful to have the psudo-C comment like other tests.


================
Comment at: llvm/test/Transforms/LoopIdiom/memset-runtime.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; REQUIRES: asserts
+; RUN: opt -passes="function(loop(loop-idiom,loop-deletion),simplifycfg)" -S < %s | FileCheck %s
----------------
Whitney wrote:
> Why do we need assert for this test case?
This one doesn't require asserts.


================
Comment at: llvm/test/Transforms/LoopIdiom/memset-runtime.ll:51
+; The C code to generate this testcase:
+; void test(int n, int m, int *ar)
+; {
----------------
`ar` is the first parameter in the IR below


================
Comment at: llvm/test/Transforms/LoopIdiom/memset-runtime.ll:59
+; }
+define void @For_NegativeStride(i32* %0, i32 %1, i32 %2) {
+; CHECK-LABEL: @For_NegativeStride(
----------------
could you rename  `%1` and `%2` to `%n` and `%m` and  update the CHECKS?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107353