[PATCH] [msan] Instrument x86.*_cvt* intrinsics.

[Evgeniy Stepanov]

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:472
@@ -471,3 +471,3 @@
   ValueMap<Value*, Value*> ShadowMap, OriginMap;
   bool InsertChecks;
   bool LoadShadow;
----------------
Alexander Potapenko wrote:
> Can there please be comments for these boolean flags. They look important.
done

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:570
@@ +569,3 @@
+      if (!isa<Instruction>(Cmp)) {
+        Cmp->dump();
+      }
----------------
Alexander Potapenko wrote:
> Please remove the debugging code.
done

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:907
@@ -903,3 +906,3 @@
     InstrumentationList.push_back(
       ShadowOriginAndInsertPoint(Shadow, Origin, OrigIns));
   }
----------------
Alexander Potapenko wrote:
> 4-space indentation here.
done

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:910
@@ -906,1 +909,3 @@
 
+  /// \brief Remember the place where a shadow check should be inserted.
+  ///
----------------
Alexander Potapenko wrote:
> The brief comments for insertCheck and insertCheckForShadow are similar - is this done on purpose?
Yes, they do basically the same thing. I've renamed them both to insertShadowCheck

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:914
@@ +913,3 @@
+  /// UMR warning in runtime if the value is not fully defined.
+  void insertCheck(Value *Val, Instruction *OrigIns) {
+    assert(Val);
----------------
Alexander Potapenko wrote:
> I dislike the name "insertCheck", especially because the function actually inserts the check for shadow. Perhaps it's really better to have two versions of insertCheckForShadow() with different arguments.
> 
> Moreover, there already is an "InsertChecks" flag.
done

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:1702
@@ +1701,3 @@
+  // CopyOp.
+  // In most cases conversion involves floating-point value which may trigger an
+  // hardware exception when not fully initialized. For this reason we require
----------------
Alexander Potapenko wrote:
> "a hardware exception"
done

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:1707
@@ +1706,3 @@
+  // without \p CopyOp always return a fully initialized value.
+  void handleVectorConvert(IntrinsicInst &I, int Used) {
+    IRBuilder<> IRB(&I);
----------------
Alexander Potapenko wrote:
> I suggest "Intrinsic" to be present in the function name (i.e. "handleVectorConvertIntrinsic").
> Also, "Used" is a poor name for an int argument. "NumUsed", maybe? (or even "NumUsedElements")
done

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:1711
@@ +1710,3 @@
+
+    if (I.getNumArgOperands() == 2) {
+      CopyOp = I.getArgOperand(0);
----------------
Alexander Potapenko wrote:
> Why not write a switch here?
done

================
Comment at: test/Instrumentation/MemorySanitizer/vector_cvt.ll:10
@@ +9,3 @@
+
+; Single argument vector convertion.
+
----------------
Alexander Potapenko wrote:
> "conversion"
done


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