SelectionDAG: Fix bug in ClusterNeighboringLoads

[nhaehnle]

Summary

The method attempts to find loads that can be legally clustered by
looking for loads consuming the same chain glue token.

However, the old code looks at _all_ users of values produced by the
chain node -- including uses of the loaded/returned value of volatile
loads or atomics. This could lead to circular dependencies which then
failed during scheduling.

With this change, we filter out users by getResNo, i.e. by which
SDValue value they use, to ensure that we only look at users of the
chain glue token.

This appears to be a rather old bug, which is perhaps surprising.
However, the test case is actually quite fragile (i.e., it is hidden
by fairly small changes), and the test _must_ use volatile loads for
the bug to manifest.

[foad]

LGTM.

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

238

Is there no slicker way of iterating the uses of an SDValue, without having to manually check getResNo() each time?

[nhaehnle]

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

238

I looked for it, but apparently not :/

llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp

 }
 
 /// ClusterNeighboringLoads - Force nearby loads together by "gluing" them.
 /// This function finds loads of the same base and different offsets. If the
 /// offsets are not far apart (target specific), it add MVT::Glue inputs and
 /// outputs to ensure they are scheduled together and in order. This
 /// optimization may benefit some targets by improving cache locality.
 void ScheduleDAGSDNodes::ClusterNeighboringLoads(SDNode *Node) {
-  SDNode *Chain = nullptr;
+  SDValue Chain;
   unsigned NumOps = Node->getNumOperands();
   if (Node->getOperand(NumOps-1).getValueType() == MVT::Other)
-    Chain = Node->getOperand(NumOps-1).getNode();
+    Chain = Node->getOperand(NumOps-1);
   if (!Chain)
     return;
 
   // Skip any load instruction that has a tied input. There may be an additional
   // dependency requiring a different order than by increasing offsets, and the
   // added glue may introduce a cycle.
   auto hasTiedInput = [this](const SDNode *N) {
     const MCInstrDesc &MCID = TII->get(N->getMachineOpcode());
void ScheduleDAGSDNodes::ClusterNeighboringLoads(SDNode *Node) {
   if (hasTiedInput(Base))
     return;
 
   // This algorithm requires a reasonably low use count before finding a match
   // to avoid uselessly blowing up compile time in large blocks.
   unsigned UseCount = 0;
   for (SDNode::use_iterator I = Chain->use_begin(), E = Chain->use_end();
        I != E && UseCount < 100; ++I, ++UseCount) {
+    if (I.getUse().getResNo() != Chain.getResNo())
+      continue;
+
     SDNode *User = *I;
     if (User == Node || !Visited.insert(User).second)
       continue;
     int64_t Offset1, Offset2;
     if (!TII->areLoadsFromSameBasePtr(Base, User, Offset1, Offset2) ||
         Offset1 == Offset2 ||
         hasTiedInput(User)) {
       // FIXME: Should be ok if they addresses are identical. But earlier

llvm/test/CodeGen/AMDGPU/bug-sdag-scheduler-cycle.ll

+; RUN: llc < %s -mtriple=amdgcn--amdpal -mcpu=gfx1010 -verify-machineinstrs | FileCheck %s --check-prefixes=CHECK
+
+; This used to cause a circular chain dependency during
+; SelectionDAG instruction scheduling.
+
+; CHECK-LABEL: {{^}}_amdgpu_gs_main:
+; CHECK: ds_read_b32
+; CHECK: ds_read_b32
+; CHECK: ds_read_b32
+; CHECK: ds_read_b32
+define amdgpu_gs float @_amdgpu_gs_main(i8 addrspace(3)* %arg0, i8 addrspace(3)* %arg1, i8 addrspace(3)* %arg2) #0 {
+  %tmp0 = bitcast i8 addrspace(3)* %arg0 to i32 addrspace(3)* addrspace(3)*
+  %tmp = load volatile i32 addrspace(3)*, i32 addrspace(3)* addrspace(3)* %tmp0, align 4
+
+  %tmp3 = load volatile i32, i32 addrspace(3)* %tmp, align 4
+
+  %tmp4a = bitcast i8 addrspace(3)* %arg1 to i32 addrspace(3)*
+  %tmp4 = load volatile i32, i32 addrspace(3)* %tmp4a, align 4
+
+  %tmp7a = getelementptr i32, i32 addrspace(3)* %tmp, i32 8
+  %tmp8 = load volatile i32, i32 addrspace(3)* %tmp7a, align 4
+
+  %tmp9 = add i32 %tmp3, %tmp8
+  %tmp10 = add i32 %tmp9, %tmp4
+  %tmp14 = bitcast i32 %tmp10 to float
+  ret float %tmp14
+}