[PATCH] Don't speculate loads under ThreadSanitizer

[Chandler Carruth]

  The actual substance of the change LGTM. Comments are nits and cleanup to polish the test case mostly.


================
Comment at: lib/Analysis/ValueTracking.cpp:2009-2012
@@ -2008,3 +2008,6 @@
     const LoadInst *LI = cast<LoadInst>(Inst);
     if (!LI->isUnordered())
       return false;
+    // Speculating a load may create a race that did not exist in the source.
+    if (LI->getParent()->getParent()->hasFnAttribute(Attribute::SanitizeThread))
+      return false;
----------------
No need for two conditions here, just put another branch in the first one.

================
Comment at: test/Transforms/SimplifyCFG/no_speculative_loads_with_tsan.ll:17
@@ +16,3 @@
+}
+; CHECK: define i32 @TestNoTsan
+; CHECK: select
----------------
You should use the new CHECK-LABEL FileCheck feature to check each function's output.

I also like to put the CHECKs inside of the function body. Either at the top (as what I expect) or immediately after the instructions. Minor point though.

================
Comment at: test/Transforms/SimplifyCFG/no_speculative_loads_with_tsan.ll:18
@@ +17,3 @@
+; CHECK: define i32 @TestNoTsan
+; CHECK: select
+; CHECK: ret i32
----------------
I would check that there is in fact a load prior to the select, and that the loaded value is an input to the select.


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

BRANCH
  svn

ARCANIST PROJECT
  llvm