-
Notifications
You must be signed in to change notification settings - Fork 21
Description
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.
Context
From @jrbyrnes:
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 forScheduleDAGOptSched
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.
@Quincunx271's reply is the rest that follows:
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.
Originally posted by @Quincunx271 in #162 (comment)