[PATCH] D18546: Prevent X86IselLowering from merging volatile loads

[Sanjay Patel via llvm-commits]

spatel added a comment.

Is there any way to expose this bug in a test case before applying http://reviews.llvm.org/D14384?


================
Comment at: include/llvm/CodeGen/SelectionDAG.h:1287
@@ -1286,2 +1286,3 @@
 
-  /// Return true if LD is loading 'Bytes' bytes from a location that is 'Dist'
+  /// areNonVolatileConsecutiveLoads - Return true loads are next to
+  /// each other and can be merged. Check that both are nonvolatile and
----------------
Don't repeat the function name in the documentation comment.
"true loads" -> "true if loads" 

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6962-6965
@@ -6961,7 +6961,6 @@
 
-
-/// isConsecutiveLoad - Return true if LD is loading 'Bytes' bytes from a
-/// location that is 'Dist' units away from the location that the 'Base' load
-/// is loading from.
-bool SelectionDAG::isConsecutiveLoad(LoadSDNode *LD, LoadSDNode *Base,
-                                     unsigned Bytes, int Dist) const {
+/// areNonVolatileConsecutiveLoads - Return true loads are next to
+/// each other and can be merged. Check that both are nonvolatile and
+/// if LD is loading 'Bytes' bytes from a location that is 'Dist'
+/// units away from the location that the 'Base' load is loading from.
+bool SelectionDAG::areNonVolatileConsecutiveLoads(LoadSDNode *LD,
----------------
jyknight wrote:
> The coding style changed somewhat recently to not have the name of the function repeated at the beginning of the doc. So, if you're changing a function with that, you can/should remove it.
You can delete the whole comment block from the implementation file. The doxygen should be generated from the header file, and users should be using that as the interface.
http://llvm.org/docs/CodingStandards.html#doxygen-use-in-documentation-comments

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:6970-6971
@@ -6968,1 +6969,4 @@
+                                                  int Dist) const {
+  if (LD->isVolatile() != Base->isVolatile())
+    return false;
   if (LD->getChain() != Base->getChain())
----------------
jyknight wrote:
> Um, you mean to check if both are !isVolatile, not if their volatility is equal, right?
Shouldn't this check if *either* load is volatile? Ie, we'll get past this check if both are volatile.


http://reviews.llvm.org/D18546