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

[Jay Foad via Phabricator via llvm-commits]

foad accepted this revision.
foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIMachineScheduler.cpp:1141
-#endif
-        SubGraph = DAG->GetTopo()->GetSubGraph(DAG->SUnits[j], SU,
-                                               HasSubGraph);
----------------
jsilvanus wrote:
> 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.
> 
Thanks for explaining!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124111