You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Currently, MachineModel can be constructed either from a file or from a string. However, conceptually, it is just a pure data object. As such, it would be better if it was a pure data object with an external function that could parse it from file or string.
However, it's a little more complicated than that. In practice, refactoring it to be pure data is a difficult task (I attempted it previously). Whatever attempted refactors are done should be done with care so that each refactor is incremental; changing large swathes of the codebase at once is infeasible both for the person performing the refactor and for any reviewers. In particular, MachineModel shouldn't be refactored into a plain old struct, as the accessors abstract away several concepts in the struct, including the differences between similar concepts (e.g. do I go down model.registerTypes or model.registers?) and giving names to things (e.g. model.regInfo.types.size() -> model.NumRegisterTypes() can be worthwhile). Even if the plain struct method would work, it requires changing too much calling code.
A second idea has to do with the runtime config of MachineModel. There is no use case to be able to change it at runtime, within our research. As such, the default model could be embedded as a const MachineModel in the code, perhaps with a couple alternatives within that could be selected.
It seems to me that in order to get the most use out of this feature, we would want the model being tested to refer to the same model we are using. However, it looks that in this PR we still load the machine model from a file when constructing ScheduleDAGOptSched, therefore in order to test and run the same machine model, we would need to maintain two configs. The immediate solution that comes to mind is to construct the MM for ScheduleDAGOptSched as done in simple_machine_model_test.cpp and removing the machine model config file altogether. This, though, leads to a less friendly UI to changing machine model params (e.g. involves re-compilation), and may not be worth implementing.
That's an interesting and quite good idea, which is out of scope for this PR. The idea here is to make as small of a change as possible in order to enable unit tests.
These unit tests often won't care about the machine model, so having a simpleMachineModel() that can be entirely different from the actual machine model can make sense. Other unit tests might care about the machine model, but then they would probably want to be able to control it to test the interaction of a feature with the machine model.
I really do like the idea, though, something like:
const MachineModel DefaultMachineModel = {
// Whatever struct initializer needed to set this up
};
We could make it so that the file is still possible, e.g. via a commandline flag, by having a parser that outputs this MachineModel. But any changes to the default model would be in the code. And common alternative models can also be builtin to the code.
Currently,
MachineModel
can be constructed either from a file or from a string. However, conceptually, it is just a pure data object. As such, it would be better if it was a pure data object with an external function that could parse it from file or string.However, it's a little more complicated than that. In practice, refactoring it to be pure data is a difficult task (I attempted it previously). Whatever attempted refactors are done should be done with care so that each refactor is incremental; changing large swathes of the codebase at once is infeasible both for the person performing the refactor and for any reviewers. In particular,
MachineModel
shouldn't be refactored into a plain old struct, as the accessors abstract away several concepts in the struct, including the differences between similar concepts (e.g. do I go downmodel.registerTypes
ormodel.registers
?) and giving names to things (e.g.model.regInfo.types.size()
->model.NumRegisterTypes()
can be worthwhile). Even if the plain struct method would work, it requires changing too much calling code.A second idea has to do with the runtime config of MachineModel. There is no use case to be able to change it at runtime, within our research. As such, the default model could be embedded as a
const MachineModel
in the code, perhaps with a couple alternatives within that could be selected.Context
From @jrbyrnes:
@Quincunx271's reply is the rest that follows:
Originally posted by @Quincunx271 in #162 (comment)
The text was updated successfully, but these errors were encountered: