Prevent X86IselLowering from merging volatile loads

[niravd]

Summary

In EltsFromConsecutiveLoads we check to see if a vector of values coming
from a set of loads can be merged into a single large load, but did not
check that the loads are non-volatile.

This issue is exposed by the Merge Consecutive Store cleanups in D14834

[RKSimon]

I'm not sure if this is necessary - we already check that we're not merging consecutive loads where any/all the loads are volatiles. DAG.isConsecutiveLoad handles these cases for us (and its why we already have tests for them) - we shouldn't need to check again in EltsFromConsecutiveLoads.

If you wish to add additional volatile load tests that'd be great; but please note, we auto-generate the merge-consecutive-loads-*.ll test files (something we're trying to encourage) - manually editting these files with entwined DAG checks is not easy to maintain.

[niravd]

I'm not sure if this is necessary - we already check that we're not merging consecutive loads where any/all the loads are volatiles. DAG.isConsecutiveLoad handles these cases for us (and its why we already have tests for them) - we shouldn't need to check again in EltsFromConsecutiveLoads.

This is currently not caught by the current tests, but if you apply D18062 and D14834 which clean up Merging of Consecutive Stores (and willl hopefully be commited soon). merge-consecutive-loads-128.ll will produce wrong code. Specifically in "merge_4f32_f32_2345_volatile", there are 4 consecutive loads with the first being volatile preventing merging them but are merged into a single volatile load (this happens in the legalize-types of SelectionDAGISel after dag-combine1).

It doesn't look like isConsecutiveLoad does any checking for volatiles though perhaps it should (though we should rename it to reflect that).

We should also probably dictate that the newLd is always non-volatile.

If you wish to add additional volatile load tests that'd be great; but please note, we auto-generate the merge-consecutive-loads-*.ll test files (something we're trying to encourage) - manually editting these files with entwined DAG checks is not easy to maintain.

Good point. I'll undo these test changes and rerun the generation for D14834 when behavior changes.

[jyknight]

In D18546#386204, @RKSimon wrote:

I'm not sure if this is necessary - we already check that we're not merging consecutive loads where any/all the loads are volatiles. DAG.isConsecutiveLoad handles these cases for us (and its why we already have tests for them) - we shouldn't need to check again in EltsFromConsecutiveLoads.

Except, it doesn't actually check that.

The only reason why "load volatile; load; load; load" is not being merged by that code already is the incoming Chain must match on the nodes for isConsecutiveLoad to return true. With the current code, the chains do not get rearranged to the proper shape.

With the new code, they do: all 4 loads can be determined to not alias, and thus they are all given the same incoming Chain (EntryToken). This is correct. Then, isConsecutiveLoad says yes.

[spatel]

In D18546#386892, @jyknight wrote:

With the new code, they do: all 4 loads can be determined to not alias, and thus they are all given the same incoming Chain (EntryToken). This is correct. Then, isConsecutiveLoad says yes.

In D18546#386817, @niravd wrote:

This is currently not caught by the current tests, but if you apply D18062 and D14834 which clean up Merging of Consecutive Stores (and willl hopefully be commited soon). merge-consecutive-loads-128.ll will produce wrong code. Specifically in "merge_4f32_f32_2345_volatile", there are 4 consecutive loads with the first being volatile preventing merging them but are merged into a single volatile load (this happens in the legalize-types of SelectionDAGISel after dag-combine1).

First, thanks to Nirav and James for working on this.

When I looked at the behavior of volatiles in relation to load merging, it looked to me like a volatile access was always guaranteed to have a different chain than a non-volatile access. Therefore, because SelectionDAG::isConsecutiveLoad() bails out when the chains are not equal, I thought we were safe. James's comment clears up that misconception.

But the test that you referenced raises a question about volatile semantics (at least to me since I don't have much experience here). Here's the test case for reference:

define <4 x float> @merge_4f32_f32_2345_volatile(float* %ptr) nounwind uwtable noinline ssp {
  %ptr0 = getelementptr inbounds float, float* %ptr, i64 2
  %ptr1 = getelementptr inbounds float, float* %ptr, i64 3
  %ptr2 = getelementptr inbounds float, float* %ptr, i64 4
  %ptr3 = getelementptr inbounds float, float* %ptr, i64 5
  %val0 = load volatile float, float* %ptr0                    <--- DANGER!
  %val1 = load float, float* %ptr1
  %val2 = load float, float* %ptr2
  %val3 = load float, float* %ptr3
  %res0 = insertelement <4 x float> undef, float %val0, i32 0
  %res1 = insertelement <4 x float> %res0, float %val1, i32 1
  %res2 = insertelement <4 x float> %res1, float %val2, i32 2
  %res3 = insertelement <4 x float> %res2, float %val3, i32 3
  ret <4 x float> %res3
}

And here's the resulting codegen after applying D18062 and D14834:

vmovss	8(%rdi), %xmm0       <--- this is the load volatile float specified by the IR
vmovups	8(%rdi), %xmm0       <--- this is a 16-byte merged load that includes the volatile region

We still have the volatile access specified by the IR. I'm guessing this is wrong though because we've accessed the volatile region a 2nd time. The LangRef seems vague to me about this:
http://llvm.org/docs/LangRef.html#volatile-memory-accesses

Assuming the extra load of the volatile region is wrong, I would prefer that the check for volatility goes in isConsecutiveLoad(). It could be triggered by an optional bool param. If we only check volatility in EltsFromConsecutiveLoads(), I'm scared that other targets may have already mutated that code and wouldn't get the fix.

[jyknight]

And here's the resulting codegen after applying D18062 and D14834:

vmovss	8(%rdi), %xmm0       <--- this is the load volatile float specified by the IR
vmovups	8(%rdi), %xmm0       <--- this is a 16-byte merged load that includes the volatile region

We still have the volatile access specified by the IR. I'm guessing this is wrong though because we've accessed the volatile region a 2nd time. The LangRef seems vague to me about this:
http://llvm.org/docs/LangRef.html#volatile-memory-accesses

I think that code is wrong because the *value* is coming from the second load, not the first. The code says to do a 4-byte volatile read of the pointer, and so it should do exactly that, and use that value, not something else. The 4-byte load is only being preserved through the chain.

Of course, at least in some circumstances, it's okay to widen a normal load to cover memory that wasn't explicitly loaded (e.g. with undef in the middle of the vector). And obviously, if it was okay to widen a normal load *without* the volatile load being present, adding a volatile load in the middle wouldn't make that transform suddenly invalid. So, with a slightly different test case (e.g. if the second pointer was volatile), you could potentially validly generate a merged 16-byte load that overlapped the volatile region, a separate 4-byte volatile load, and then combine those to the result vector. But even so

Assuming the extra load of the volatile region is wrong, I would prefer that the check for volatility goes in isConsecutiveLoad(). It could be triggered by an optional bool param. If we only check volatility in EltsFromConsecutiveLoads(), I'm scared that other targets may have already mutated that code and wouldn't get the fix.

That makes sense to me. The only other caller in-tree of this function already checks isVolatile, anyways.

[spatel]

In D18546#387143, @jyknight wrote:

That makes sense to me. The only other caller in-tree of this function already checks isVolatile, anyways.

Thanks for checking that. Note that the PPC backend has this:

// Like SelectionDAG::isConsecutiveLoad, but also works for stores, and does
// not enforce equality of the chain operands.
static bool isConsecutiveLS(SDNode *N, LSBaseSDNode *Base,

...I didn't look closely, but it might have related problems.

Also, independent of volatile but maybe affected by the other patches-in-progress:
https://llvm.org/bugs/show_bug.cgi?id=10114

[niravd]

Revert tests, move check to ConsecutiveLoad. Simplify callers

[niravd]

It looks like the bug (https://llvm.org/bugs/show_bug.cgi?id=10114) is easy to fix and I have a patch that appears to fix an equivalent problem in merge-consecutive-loads-128.ll with D14834 and D18062, but not this patch. However, the example in the bug doesn't exercise this anymore so I'll do it separately after I cons up.

[niravd]

Apply clang-format

[jyknight]

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6962 ↗

(On Diff #52105)

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.

6970 ↗

(On Diff #52105)

Um, you mean to check if both are !isVolatile, not if their volatility is equal, right?

[spatel]

Is there any way to expose this bug in a test case before applying D14384?

include/llvm/CodeGen/SelectionDAG.h

1287 ↗

(On Diff #52105)

Don't repeat the function name in the documentation comment.
"true loads" -> "true if loads"

lib/CodeGen/SelectionDAG/SelectionDAG.cpp

6962–6965 ↗

(On Diff #52105)

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

6970–6971 ↗

(On Diff #52105)

Shouldn't this check if *either* load is volatile? Ie, we'll get past this check if both are volatile.

[niravd]

remove embarassing bug and fix comments

[RKSimon]

LGTM - couple of minors.

PPCISelLowering refers to SelectionDAG::isConsecutiveLoad in a comment, please can you update it so that it still makes sense?

lib/CodeGen/SelectionDAG/DAGCombiner.cpp

7257 ↗

(On Diff #52168)

This is really horrid to grok - please can you pull out 'LD1VT.getSizeInBits() / 8' as a 'LD1Bytes' or something so we can have the call on one line?

llvm/trunk/include/llvm/CodeGen/SelectionDAG.h

#endif
   /// Utility function used by legalize and lowering to
   /// "unroll" a vector operation by splitting out the scalars and operating
   /// on each element individually.  If the ResNE is 0, fully unroll the vector
   /// op. If ResNE is less than the width of the vector op, unroll up to ResNE.
   /// If the  ResNE is greater than the width of the vector op, unroll the
   /// vector op and fill the end of the resulting vector with UNDEFS.
   SDValue UnrollVectorOp(SDNode *N, unsigned ResNE = 0);
 
-  /// 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 isConsecutiveLoad(LoadSDNode *LD, LoadSDNode *Base,
+  /// Return true if 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 areNonVolatileConsecutiveLoads(LoadSDNode *LD, LoadSDNode *Base,
                                       unsigned Bytes, int Dist) const;
 
   /// Infer alignment of a load / store address. Return 0 if
   /// it cannot be inferred.
   unsigned InferPtrAlignment(SDValue Ptr) const;
 
   /// Compute the VTs needed for the low/hi parts of a type
   /// which is split (or expanded) into two not necessarily identical pieces.
   std::pair<EVT, EVT> GetSplitDestVTs(const EVT &VT) const;

llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

SDValue DAGCombiner::CombineConsecutiveLoads(SDNode *N, EVT VT) {
   assert(N->getOpcode() == ISD::BUILD_PAIR);
 
   LoadSDNode *LD1 = dyn_cast<LoadSDNode>(getBuildPairElt(N, 0));
   LoadSDNode *LD2 = dyn_cast<LoadSDNode>(getBuildPairElt(N, 1));
   if (!LD1 || !LD2 || !ISD::isNON_EXTLoad(LD1) || !LD1->hasOneUse() ||
       LD1->getAddressSpace() != LD2->getAddressSpace())
     return SDValue();
   EVT LD1VT = LD1->getValueType(0);
-
-  if (ISD::isNON_EXTLoad(LD2) &&
-      LD2->hasOneUse() &&
-      // If both are volatile this would reduce the number of volatile loads.
-      // If one is volatile it might be ok, but play conservative and bail out.
-      !LD1->isVolatile() &&
-      !LD2->isVolatile() &&
-      DAG.isConsecutiveLoad(LD2, LD1, LD1VT.getSizeInBits()/8, 1)) {
+  unsigned LD1Bytes = LD1VT.getSizeInBits() / 8;
+  if (ISD::isNON_EXTLoad(LD2) && LD2->hasOneUse() &&
+      DAG.areNonVolatileConsecutiveLoads(LD2, LD1, LD1Bytes, 1)) {
     unsigned Align = LD1->getAlignment();
     unsigned NewAlign = DAG.getDataLayout().getABITypeAlignment(
         VT.getTypeForEVT(*DAG.getContext()));
 
     if (NewAlign <= Align &&
         (!LegalOperations || TLI.isOperationLegal(ISD::LOAD, VT)))
       return DAG.getLoad(VT, SDLoc(N), LD1->getChain(),
                          LD1->getBasePtr(), LD1->getPointerInfo(),

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp

SDValue SelectionDAG::UnrollVectorOp(SDNode *N, unsigned ResNE) {
 
   for (; i < ResNE; ++i)
     Scalars.push_back(getUNDEF(EltVT));
 
   return getNode(ISD::BUILD_VECTOR, dl,
                  EVT::getVectorVT(*getContext(), EltVT, ResNE), Scalars);
 }
 
-
-/// 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 {
+bool SelectionDAG::areNonVolatileConsecutiveLoads(LoadSDNode *LD,
+                                                  LoadSDNode *Base,
+                                                  unsigned Bytes,
+                                                  int Dist) const {
+  if (LD->isVolatile() || Base->isVolatile())
+    return false;
   if (LD->getChain() != Base->getChain())
     return false;
   EVT VT = LD->getValueType(0);
   if (VT.getSizeInBits() / 8 != Bytes)
     return false;
 
   SDValue Loc = LD->getOperand(1);
   SDValue BaseLoc = Base->getOperand(1);

llvm/trunk/lib/Target/X86/X86ISelLowering.cpp

static SDValue EltsFromConsecutiveLoads(EVT VT, ArrayRef<SDValue> Elts,
   // Consecutive loads with UNDEFs and ZEROs elements require a
   // an additional shuffle stage to clear the ZERO elements.
   bool IsConsecutiveLoad = true;
   bool IsConsecutiveLoadWithZeros = true;
   for (int i = FirstLoadedElt + 1; i <= LastLoadedElt; ++i) {
     if (LoadMask[i]) {
       SDValue Elt = peekThroughBitcasts(Elts[i]);
       LoadSDNode *LD = cast<LoadSDNode>(Elt);
-      if (!DAG.isConsecutiveLoad(LD, LDBase,
-                                 Elt.getValueType().getStoreSizeInBits() / 8,
-                                 i - FirstLoadedElt)) {
+      if (!DAG.areNonVolatileConsecutiveLoads(
+              LD, LDBase, Elt.getValueType().getStoreSizeInBits() / 8,
         IsConsecutiveLoad = false;
         IsConsecutiveLoadWithZeros = false;
         break;
       }
     } else if (ZeroMask[i]) {
       IsConsecutiveLoad = false;
     }
   }
 
   auto CreateLoad = [&DAG, &DL](EVT VT, LoadSDNode *LDBase) {
-    SDValue NewLd = DAG.getLoad(VT, DL, LDBase->getChain(),
-                                LDBase->getBasePtr(), LDBase->getPointerInfo(),
-                                LDBase->isVolatile(), LDBase->isNonTemporal(),
-                                LDBase->isInvariant(), LDBase->getAlignment());
+    SDValue NewLd = DAG.getLoad(
+        VT, DL, LDBase->getChain(), LDBase->getBasePtr(),
+        LDBase->getPointerInfo(), false /*LDBase->isVolatile()*/,
+        LDBase->isNonTemporal(), LDBase->isInvariant(), LDBase->getAlignment());
 
     if (LDBase->hasAnyUseOfValue(1)) {
       SDValue NewChain =
           DAG.getNode(ISD::TokenFactor, DL, MVT::Other, SDValue(LDBase, 1),
                       SDValue(NewLd.getNode(), 1));
       DAG.ReplaceAllUsesOfValueWith(SDValue(LDBase, 1), NewChain);
       DAG.UpdateNodeOperands(NewChain.getNode(), SDValue(LDBase, 1),
                              SDValue(NewLd.getNode(), 1));