[PATCH] D124111: [AMDGPU]: Fix failing assertion in SIMachineScheduler

[Jannik Silvanus via Phabricator via llvm-commits]

jsilvanus marked 2 inline comments as done.
jsilvanus added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineScheduler.cpp:1141
-#endif
-        SubGraph = DAG->GetTopo()->GetSubGraph(DAG->SUnits[j], SU,
-                                               HasSubGraph);
----------------
foad wrote:
> Can you confirm that these subgraph-based checks are no longer required? Is that because your new check is stronger?
**Regarding the assertion check: **The subgraph-based assertion is indeed dropped in the new version and no longer explicitly checked. 
But it is implied by the topological order, and actually the new implementation no longer depends on the topological order to be correct, because we just check for the existence
of some edge from an export to a non-export.

**Regarding the second subgraph-based check not guarded by `NDEBUG`**: Yes and yes.

The old test determines the set of instructions reachable from `DAG->SUnits[j]` from which `SU` is reachable, using the `GetSubGraph` call,
and aborts if any of them is a non-export.
The new test checks if there is any non-export directly reachable from `SU`.

Because the new test goes the other direction, the test is incomparable for a particular `SU` object. But when considering the whole for loop,
the new test is stronger: If there is any pair  of `DAG->SUnits[j]` and `SU` for which the old check aborts, then there is a path from an export to an export
that visits a non-export. On that path, there must be an edge from an export to a non-export, and for that edge the new test will abort.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124111