[PATCH] D51751: Merge clang's isRepeatedBytePattern with LLVM's isBytewiseValue

[JF Bastien via Phabricator via llvm-commits]

jfb added inline comments.


================
Comment at: lib/Analysis/ValueTracking.cpp:3121
+
+  if (isa<ConstantArray>(C) || isa<ConstantStruct>(C)) {
+    Value *Val = UndefInt8;
----------------
efriedma wrote:
> jfb wrote:
> > efriedma wrote:
> > > Do you really need separate loops for ConstantVector and ConstantArray?
> > I don't think ConstantArray has getSplatValue, which I use for ConstantVector. Or are you suggesting something else?
> I meant you could just use the for loop for ConstantVector.  Maybe a bit less efficient, but the difference is unlikely to matter.
I'd rather not since `getSplatValue` does exactly what we want here.


================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:391
                                                  Value *StartPtr,
-                                                 Value *ByteVal) {
+                                                 Value *&ByteVal) {
   const DataLayout &DL = StartInst->getModule()->getDataLayout();
----------------
efriedma wrote:
> jfb wrote:
> > efriedma wrote:
> > > What's the point of changing this signature?  It doesn't seem to have any effect.
> > If ByteVal is UndefValue then it can be merged with any other valid pattern. The below change does this. The signature change allows updating ByteVal. I could instead return a pair if you want.
> Whether you use an out parameter or a pair, the value still isn't actually used by either of the callers of tryMergingIntoMemset.
> 
> Looking again, I guess technically it's used in MemCpyOptPass::processStore, but only in the case where tryMergingIntoMemset fails, so I'm guessing that isn't intentional.
You're right, changed back.


Repository:
  rL LLVM

https://reviews.llvm.org/D51751