[PATCH] Don't speculate loads under ThreadSanitizer
Chandler Carruth
chandlerc at gmail.com
Wed Nov 20 22:57:43 PST 2013
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
More information about the llvm-commits
mailing list