[AMDGPU/MemOpsCluster] Code clean-up around mem ops clustering logic

[hsmhsm]

Summary

Clean-up code around mem ops clustering logic. This patch cleans up code within
the function clusterNeighboringMemOps(). It is WIP, and this patch is a first cut.

[foad]

This causes lots of test failures:

Failing Tests (35):
  LLVM :: CodeGen/AMDGPU/GlobalISel/cvt_f32_ubyte.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.div.fmas.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.div.scale.ll
  LLVM :: CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.ubfe.ll
  LLVM :: CodeGen/AMDGPU/amdgpu-codegenprepare-idiv.ll
  LLVM :: CodeGen/AMDGPU/amdhsa-trap-num-sgprs.ll
  LLVM :: CodeGen/AMDGPU/bitreverse.ll
  LLVM :: CodeGen/AMDGPU/call-argument-types.ll
  LLVM :: CodeGen/AMDGPU/cvt_f32_ubyte.ll
  LLVM :: CodeGen/AMDGPU/fshr.ll
  LLVM :: CodeGen/AMDGPU/idot2.ll
  LLVM :: CodeGen/AMDGPU/idot4s.ll
  LLVM :: CodeGen/AMDGPU/idot4u.ll
  LLVM :: CodeGen/AMDGPU/idot8s.ll
  LLVM :: CodeGen/AMDGPU/idot8u.ll
  LLVM :: CodeGen/AMDGPU/insert_vector_dynelt.ll
  LLVM :: CodeGen/AMDGPU/insert_vector_elt.v2i16.ll
  LLVM :: CodeGen/AMDGPU/llvm.amdgcn.raw.buffer.load.ll
  LLVM :: CodeGen/AMDGPU/llvm.maxnum.f16.ll
  LLVM :: CodeGen/AMDGPU/llvm.minnum.f16.ll
  LLVM :: CodeGen/AMDGPU/llvm.round.f64.ll
  LLVM :: CodeGen/AMDGPU/memory_clause.ll
  LLVM :: CodeGen/AMDGPU/merge-stores.ll
  LLVM :: CodeGen/AMDGPU/promote-constOffset-to-imm.ll
  LLVM :: CodeGen/AMDGPU/salu-to-valu.ll
  LLVM :: CodeGen/AMDGPU/sdiv.ll
  LLVM :: CodeGen/AMDGPU/sdiv64.ll
  LLVM :: CodeGen/AMDGPU/setcc-limit-load-shrink.ll
  LLVM :: CodeGen/AMDGPU/sgpr-control-flow.ll
  LLVM :: CodeGen/AMDGPU/smrd.ll
  LLVM :: CodeGen/AMDGPU/srem64.ll
  LLVM :: CodeGen/AMDGPU/trunc-combine.ll
  LLVM :: CodeGen/AMDGPU/udiv64.ll
  LLVM :: CodeGen/AMDGPU/urem64.ll
  LLVM :: CodeGen/AMDGPU/use-sgpr-multiple-times.ll


Testing Time: 358.21s
  Unsupported Tests  :   102
  Expected Passes    : 15421
  Expected Failures  :    49
  Unexpected Failures:    35

[foad]

(1) NumLoads argument can initially be set to 2 instead of 1, this avoids incrementing NumLoads while calling shouldClusterMemOps()

Alternatively you could move ++ClusterLength to just before the call to shouldClusterMemOps. But I really don't think this is very important.

(2) Re-arrange the code within clusterNeighboringMemOps(), so that the code is more convenient to understand

I agree that using continue is more readable.

(3) Improve the logic within shouldClusterMemOps() and remove all FIXMEs

You've changed it to accurately count bytes in a cluster, which is nice, but (a) it's a change in behvaiour so needs test updates and benchmarking, and (b) I don't particularly like the implementation. I think a cleaner way to do this would be to change all target getMemOperandsWithOffset functions to return a Width (in bytes) as well (some targets already have an internal getMemOperandsWithOffsetWidth function which does this) and change MachineScheduler to use this information to track the size of a cluster, and pass that information into shouldClusterMemOps.

[hsmhsm]

Since it is safe to do clean-up step-by-step, the previous somewhat larger
patch is reverted back, and in this modified patch, initial step is taken
towards the clean-up.

[hsmhsm]

Hi Jay,

I would not want to submit bigger patch since it is bit risky. So, let's take it step-by-step at time. This modified patch is the first cut.

[foad]

Looks OK to me. Please also update the "summary" section at the top of this review (unfortunately "arc diff" does not do this for you when you edit the git commit message, so you have to do it by hand).

llvm/lib/CodeGen/MachineScheduler.cpp

#ifndef NDEBUG
     for (auto *Op : BaseOps)
       assert(Op);
 #endif
   }
   if (MemOpRecords.size() < 2)
     return;
 
   llvm::sort(MemOpRecords);
+
+  // At this point, `MemOpRecords` array must hold atleast two mem ops. Try to
+  // cluster mem ops collected within `MemOpRecords` array.
   unsigned ClusterLength = 1;
   for (unsigned Idx = 0, End = MemOpRecords.size(); Idx < (End - 1); ++Idx) {
-    SUnit *SUa = MemOpRecords[Idx].SU;
-    SUnit *SUb = MemOpRecords[Idx+1].SU;
-    if (TII->shouldClusterMemOps(MemOpRecords[Idx].BaseOps,
-                                 MemOpRecords[Idx + 1].BaseOps,
-                                 ClusterLength + 1)) {
-      if (SUa->NodeNum > SUb->NodeNum)
-        std::swap(SUa, SUb);
-      if (DAG->addEdge(SUb, SDep(SUa, SDep::Cluster))) {
-        LLVM_DEBUG(dbgs() << "Cluster ld/st SU(" << SUa->NodeNum << ") - SU("
-                          << SUb->NodeNum << ")\n");
-        // Copy successor edges from SUa to SUb. Interleaving computation
-        // dependent on SUa can prevent load combining due to register reuse.
-        // Predecessor edges do not need to be copied from SUb to SUa since
-        // nearby loads should have effectively the same inputs.
-        for (const SDep &Succ : SUa->Succs) {
-          if (Succ.getSUnit() == SUb)
-            continue;
-          LLVM_DEBUG(dbgs()
-                     << "  Copy Succ SU(" << Succ.getSUnit()->NodeNum << ")\n");
-          DAG->addEdge(Succ.getSUnit(), SDep(SUb, SDep::Artificial));
-        }
-        ++ClusterLength;
-      } else
-        ClusterLength = 1;
-    } else
-      ClusterLength = 1;
+    // Decision to cluster mem ops is taken based on target dependent logic
+    auto MemOpa = MemOpRecords[Idx];
+    auto MemOpb = MemOpRecords[Idx + 1];
+    ++ClusterLength;
+    if (!TII->shouldClusterMemOps(MemOpa.BaseOps, MemOpb.BaseOps,
+                                  ClusterLength)) {
+      // Current mem ops pair could not be clustered, reset cluster length, and
+      // go to next pair
+      ClusterLength = 1;
+      continue;
+    }
+
+    SUnit *SUa = MemOpa.SU;
+    SUnit *SUb = MemOpb.SU;
+
+    // FIXME: Is this check really required?
+    if (!DAG->addEdge(SUb, SDep(SUa, SDep::Cluster))) {
+      ClusterLength = 1;
+      continue;
+    }
+
+
+      LLVM_DEBUG(dbgs() << "  Copy Succ SU(" << Succ.getSUnit()->NodeNum
+                        << ")\n");
   }
 }
 
 /// Callback from DAG postProcessing to create cluster edges for loads.
 void BaseMemOpClusterMutation::apply(ScheduleDAGInstrs *DAG) {
   // Map DAG NodeNum to a set of dependent MemOps in store chain.
   DenseMap<unsigned, SmallVector<SUnit *, 4>> StoreChains;
   for (SUnit &SU : DAG->SUnits) {