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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

It might be good do change the function signatures (to take the `ScalarEvolution` argument, to take a SCEV instead of `unsigned`) in a preparatory NFC patch.
Some more catch-all inline comments..



================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:220
 
-  bool processLoopStridedStore(Value *DestPtr, unsigned StoreSize,
+  bool processLoopStridedStore(Value *DestPtr, const SCEV *StoreSizeSCEV,
                                MaybeAlign StoreAlignment, Value *StoredVal,
----------------
Can this be changed in a preparatory NFC patch?


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:785-786
 
-    if (processLoopStridedStore(StorePtr, StoreSize,
+    const SCEV *StoreSizeSCEV = SE->getConstant(BECount->getType(), StoreSize);
+    if (processLoopStridedStore(StorePtr, StoreSizeSCEV,
                                 MaybeAlign(HeadStore->getAlignment()),
----------------
Can this be changed in a preparatory NFC patch?


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:917
 
-  // Check to see if the stride matches the size of the memset.  If so, then we
-  // know that every byte is touched in the loop.
-  const SCEVConstant *ConstStride = dyn_cast<SCEVConstant>(Ev->getOperand(1));
-  if (!ConstStride)
-    return false;
+  bool NegStride;
+  const bool IsConstantSize = isa<ConstantInt>(MSI->getLength());
----------------
Can you make this change globally trouough the file in a preparatory NFC patch?

(Though, somehow it feels conceptually wrong to pass it into processLoopStridedStore().)


================
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");
----------------
Negative test missing


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:953
+    }
+    if (!SE->isLoopInvariant(MemsetSizeSCEV, CurLoop) ||
+        !SE->isLoopInvariant(StrideSCEV, CurLoop)) {
----------------
Negative test missing


================
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, "
----------------
Negative test missing


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:961
+    // compare positive direction strideSCEV with MemsizeSizeSCEV
+    NegStride = StrideSCEV->isNonConstantNegative();
+    const SCEV *PositiveStrideSCEV =
----------------
Test missing


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:968
+
+    if (PositiveStrideSCEV != MemsetSizeSCEV) {
+      // TODO: folding can be done to the SCEVs
----------------
Test missing


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:993
 mayLoopAccessLocation(Value *Ptr, ModRefInfo Access, Loop *L,
-                      const SCEV *BECount, unsigned StoreSize,
+                      const SCEV *BECount, const SCEV *StoreSizeSCEV,
                       AliasAnalysis &AA,
----------------
Can this be changed in a preparatory NFC patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107353