[PATCH] D31587: MachineScheduler/ScheduleDAG: Add support for getNode2Index

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

In https://reviews.llvm.org/D31587#717117, @axeldavy wrote:

> In https://reviews.llvm.org/D31587#716821, @MatzeB wrote:
>
> > - Choose a better function name!
>
>
> getTopoIndexes ?
>
> > - Now that the function is public you should also document some facts about it like what to use as index into the vector or the range of indizes coming out of it.
>
> What about: 
>  / / / Node2Index[SU->NodeNum] (when not boundary SU) gives the topological index in the topological sort of ScheduleDAGTopologicalSort.
>  / / / The index is contained between 0 and SUnits.count()-1
>
> > - Instead of returning `const vector<int>&` better use `ArrayRef<int>` as that gives less constraints on the internal implementation.
>
> Ok
>
> > - Is it necessary to return the array as a whole or would a simpler function suffice that takes a single `const SUnit&` and returns the index into the corresponding topological ordering?
>
> No, I'd be fine with a function returning Node2Index[SU->NodeNum]. Do you think a function would be preferable ?
>  One worry is that if we write a function that takes SU and returns the index, it'd likely have to check for boundaries every call.


The function would not expose the fact that we currently have an underlying std::vector to the API (ArrayRef makes it slightly better but it still forces the implementation to use some form of array). So if the function works as well I would prefer that.

As for performance you would perform the bound checks in an `assert()` so they are not present in a release compiler.


Repository:
  rL LLVM

https://reviews.llvm.org/D31587