Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor MachineModel, removing the runtime config and leaving a pure data type #175

Open
Quincunx271 opened this issue Oct 27, 2021 · 0 comments
Labels
cleanup enhancement New feature or request

Comments

@Quincunx271
Copy link
Member

Quincunx271 commented Oct 27, 2021

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 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.

@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)

@Quincunx271 Quincunx271 added enhancement New feature or request cleanup labels Oct 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant