[PATCH] D79670: [AMDGPU] Order pos exports before param exports

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUExportClustering.cpp:38
+
+  // Move position exports before other exports while preserving
+  // the order within different export types (pos or other).
----------------
Can you say why it's beneficial to do position exports first?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUExportClustering.cpp:41
+  unsigned InsertionPoint = 0;
+  for (unsigned Idx = 0, End = Chain.size(); Idx < End; ++Idx) {
+    SUnit *SU = Chain[Idx];
----------------
It's a shame that this sorting is O(n^2) but I guess it's not a problem because the average chain length will be 2?

You could probably do this sorting in a cute way with std::partition_copy if you felt inclined: https://en.cppreference.com/w/cpp/algorithm/partition_copy


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUExportClustering.cpp:82-85
+  // Pass through DAG gathering a list of exports and removing barrier edges
+  // creating dependencies on exports. Freeing exports of successor edges
+  // allows more scheduling freedom, and nothing should be order dependent
+  // on exports.  Edges will be added later to order the exports.
----------------
Why are the barrier edges there in the first place? Either the exports can be reordered, so the barrier edges should not be there; or they can't be, so we shouldn't ignore the barrier edges!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79670/new/

https://reviews.llvm.org/D79670