[llvm-commits] [PATCH] [msan] Refactor default shadow propagation and origin tracking

[Kostya Serebryany]

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:942
@@ +941,3 @@
+    IRBuilder<> &IRB;
+    MemorySanitizerVisitor* MSV;
+  public:
----------------
"Type *Foo", not "Type* Foo"

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:1012
@@ +1011,3 @@
+      return IRB.CreateIntCast(V, dstTy, false);
+    size_t srcSizeInBits = srcTy->isVectorTy() ?
+      srcTy->getVectorNumElements() * srcTy->getScalarSizeInBits() :
----------------
these 3 lines and the following 3 lines look like a candidate for a separate function

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:937
@@ +936,3 @@
+  /// arguments are initialized.
+  template <bool CombineShadow, bool CombineOrigin>
+  class Combiner {
----------------
do you ever plan to have CombineOrigin==false?


================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:958
@@ +957,3 @@
+
+      if (CombineOrigin && ClTrackOrigins) {
+        assert(OpOrigin);
----------------
I wonder if we can/should move ClTrackOrigins outside of this class (e.g. make it a CTOR argument)

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:978
@@ +977,3 @@
+
+    void Done(Instruction *I) {
+      if (CombineShadow) {
----------------
Do you need 'I' here? 
Why IRB is not enough?
Or maybe pass I instead of IRB to the CTOR?


http://llvm-reviews.chandlerc.com/D215