[PATCH] D108289: [NFC][LoopIdiom] Let processLoopStoreOfLoopLoad take StoreSize as SCEV instead of unsigned

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:891-892
 
-  return processLoopStoreOfLoopLoad(Dest, Source, (unsigned)SizeInBytes,
+  const SCEV *MemCpySizeSCEV = SE->getSCEV(MCI->getLength());
+  return processLoopStoreOfLoopLoad(Dest, Source, MemCpySizeSCEV,
                                     MCI->getDestAlign(), MCI->getSourceAlign(),
----------------
why not:
```
return processLoopStoreOfLoopLoad(Dest, Source, SE->getSCEV(SizeInBytes),
```
?


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1247
+
+  const SCEV *StoreSizeSCEV = SE->getConstant(StorePtr->getType(), StoreSize);
+  return processLoopStoreOfLoopLoad(StorePtr, LoadPtr, StoreSizeSCEV,
----------------
StorePtr->getType() is the type of the pointer, did you mean `SI->getValueOperand()->getType()`? 


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1281
+  const SCEVConstant *ConstStoreSize = dyn_cast<SCEVConstant>(StoreSizeSCEV);
+  int64_t StoreSize = ConstStoreSize->getValue()->getSExtValue();
   bool IsNegStride = StoreSize == -Stride;
----------------
this is being sign extended but SCEV being passed to `processLoopStoreOfLoopLoad` is created using `ScalarEvolution::getConstant` with the default value of `false` for the `isSigned` parameter.


================
Comment at: llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp:1281
+  const SCEVConstant *ConstStoreSize = dyn_cast<SCEVConstant>(StoreSizeSCEV);
+  int64_t StoreSize = ConstStoreSize->getValue()->getSExtValue();
   bool IsNegStride = StoreSize == -Stride;
----------------
bmahjour wrote:
> this is being sign extended but SCEV being passed to `processLoopStoreOfLoopLoad` is created using `ScalarEvolution::getConstant` with the default value of `false` for the `isSigned` parameter.
```
/// TODO: until non-constant sizes are handled
assert(ConstStoreSize && "store size is expected to be a constant");
```
or add logic to deal with non-constant situations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108289