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

[Yueh-Ting Chen via Phabricator via llvm-commits]

eopXD added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:949
+    LLVM_DEBUG(dbgs() << "  memset size is non-constant\n");
+    if (Pointer->getType()->getPointerAddressSpace() != 0) {
+      LLVM_DEBUG(dbgs() << "  pointer is not in address space zero\n");
----------------
lebedev.ri wrote:
> Negative test missing
`isLegalStore` already blocks non-zero address spaces. So this guard will never trigger and should be deleted.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:953
+    }
+    if (!SE->isLoopInvariant(MemsetSizeSCEV, CurLoop) ||
+        !SE->isLoopInvariant(StrideSCEV, CurLoop)) {
----------------
lebedev.ri wrote:
> Negative test missing
Added `memset-runtime-negative-cases.ll` - `@MemsetSize_LoopVariant`.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:954
+    if (!SE->isLoopInvariant(MemsetSizeSCEV, CurLoop) ||
+        !SE->isLoopInvariant(StrideSCEV, CurLoop)) {
+      LLVM_DEBUG(dbgs() << "  memset size or stride is not a loop-invariant, "
----------------
lebedev.ri wrote:
> Negative test missing
`isLegalStore` already blocks non-affine SCEV for the pointer in the memset instruction. So this guard will never trigger and should be deleted.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:961
+    // compare positive direction strideSCEV with MemsizeSizeSCEV
+    NegStride = StrideSCEV->isNonConstantNegative();
+    const SCEV *PositiveStrideSCEV =
----------------
lebedev.ri wrote:
> Test missing
Added `memset-runtime.ll` - `@For_NegativeStride`




================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:968
+
+    if (PositiveStrideSCEV != MemsetSizeSCEV) {
+      // TODO: folding can be done to the SCEVs
----------------
lebedev.ri wrote:
> Test missing
Added `memset-runtime-negative-cases.ll` - `@MemsetSize_Stride_Mismatch`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107353