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

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

LGTM once Eli approves.



================
Comment at: lib/Analysis/ValueTracking.cpp:3046
 Value *llvm::isBytewiseValue(Value *V) {
+  LLVMContext &Ctx = V->getContext();
+
----------------
You could move this down, closer to its first user.


================
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.
----------------
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.


================
Comment at: lib/Analysis/ValueTracking.cpp:3129
 
-  // 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.
+  // BlockAddress, ConstantExpr, and everything else is scary.
   return nullptr;
----------------
not sure how valuable this comment is :)


Repository:
  rL LLVM

https://reviews.llvm.org/D51751