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

[Alexander Potapenko]

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

================
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);
----------------
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")

================
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
----------------
"a hardware exception"

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

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

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

================
Comment at: lib/Transforms/Instrumentation/MemorySanitizer.cpp:910
@@ -906,1 +909,3 @@
 
+  /// \brief Remember the place where a shadow check should be inserted.
+  ///
----------------
The brief comments for insertCheck and insertCheckForShadow are similar - is this done on purpose?

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

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


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