[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:3059-3064
+    // Conceptually, we could handle things like:
+    //   %a = zext i8 %X to i16
+    //   %b = shl i16 %a, 8
+    //   %c = or i16 %a, %b
+    // but until there is an example that actually needs this, it doesn't seem
+    // worth worrying about.
----------------
MatzeB wrote:
> Indeed an interesting discussion whether we should call into something like InstructionSimplify here. I agree with you that we'd rather see this handled in a separate step by whoever calls `isByteWiseValue()`. That way we can keep this function simple and only handle the patterns that are already simplified.
This comment was originally at the bottom of the function, I moved it to early-exit if it's not a constant because all the subsequent code is about constants (not values).

I can drop the comment if you want?


================
Comment at: lib/Analysis/ValueTracking.cpp:3121
+
+  if (isa<ConstantArray>(C) || isa<ConstantStruct>(C)) {
+    Value *Val = UndefInt8;
----------------
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?


================
Comment at: lib/Transforms/Scalar/LoopIdiomRecognize.cpp:654
+          if (isa<UndefValue>(FirstPatternValue))
+            FirstPatternValue = SecondPatternValue;
           if (FirstPatternValue != SecondPatternValue)
----------------
efriedma wrote:
> No test coverage for this change?
Indeed, right now getMemSetPatternValue doesn't check for UndefValue so there's no way to trigger this line. I'm adding it opportunistically because it's obviously correct, and I added a FIXME in getMemSetPatternValue to do this.


================
Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:391
                                                  Value *StartPtr,
-                                                 Value *ByteVal) {
+                                                 Value *&ByteVal) {
   const DataLayout &DL = StartInst->getModule()->getDataLayout();
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D51751