[llvm-commits] [PATCH] [msan] Heuristic handling on unknown intrinsics

[Kostya Serebryany]

  The general approach sounds ok, but I don't like the code with so many loops, ifs and early returns.
  I'd like to see all existing intrinsics that we handle this way and see how many different patters are there.
  If there are just a few patters, then I'd prefer to have a sequence of ifs, each if matching a particular patter.


================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:975
@@ +974,3 @@
+    Value* V1 = IRB.CreateBitCast(V, Type::getIntNTy(*MS.C, srcSizeInBits));
+    Value* V2 = IRB.CreateIntCast(V1, Type::getIntNTy(*MS.C, dstSizeInBits), false);
+    return IRB.CreateBitCast(V2, dstTy);
----------------
80 chars

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:961
@@ +960,3 @@
+  // Cast between two shadow types, extending or truncating as necessary.
+  Value* CreateShadowCast(IRBuilder<>& IRB, Value* V, Type* dstTy) {
+    Type* srcTy = V->getType();
----------------
don't we use "Type *" in other places?

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1201
@@ +1200,3 @@
+  ///    together.
+  /// 3. If we think this intrinsics reads memory, load shadow for the base type
+  ///    of the pointer argument and bitwise OR it with the rest.
----------------
s/intrinsics/intrinsic/

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1233
@@ +1232,3 @@
+    int pointerOpIdx = -1;
+    Type* MemAccessType = NULL;
+    for (unsigned i = 0, n = I.getNumArgOperands(); i < n; ++i) {
----------------
don't we use 0 instead of NULL?

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1240
@@ +1239,3 @@
+        } else {
+          // Two pointer operands? Meh.
+          visitInstruction(I);
----------------
Does this ever happen?
Maybe add an assert for debug mode? 
In release more we'll call visitInstruction

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1249
@@ +1248,3 @@
+
+    if (pointerOpIdx < 0 && (readsMemory || writesMemory)) {
+      // No idea how to handle this.
----------------
same here. Maybe add an assert? 

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1339
@@ +1338,3 @@
+      setShadow(&I, Shadow);
+      if (ClTrackOrigins)
+        setOrigin(&I, Origin);
----------------
Do you need this if? 

================
Comment at: llvm/lib/Transforms/Instrumentation/MemorySanitizer.cpp:1331
@@ +1330,3 @@
+      Value *ShadowPtr = getShadowPtr(Op, MemAccessShadowTy, IRB);
+      IRB.CreateAlignedStore(Shadow, ShadowPtr, 1);
+      if (ClTrackOrigins)
----------------
This 'alignment 1' deserves a comment


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