[PATCH] D74253: SelectionDAG: Fix bug in ClusterNeighboringLoads

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle created this revision.
nhaehnle added reviewers: arsenm, bogner, craig.topper, foad.
Herald added subscribers: kerbowa, jfb, javed.absar, hiraditya, wdng, jvesely, MatzeB.
Herald added a project: LLVM.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74253

Files:
  llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
  llvm/test/CodeGen/AMDGPU/bug-sdag-scheduler-cycle.ll


Index: llvm/test/CodeGen/AMDGPU/bug-sdag-scheduler-cycle.ll
===================================================================
--- /dev/null
+++ llvm/test/CodeGen/AMDGPU/bug-sdag-scheduler-cycle.ll
@@ -0,0 +1,27 @@
+; 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
+}
Index: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
===================================================================
--- llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
+++ llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp
@@ -198,10 +198,10 @@
 /// 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;
 
@@ -234,6 +234,9 @@
   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;


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D74253.243273.patch
Type: text/x-patch
Size: 2384 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200207/7b4bfd79/attachment.bin>