[snippy] Dump initial registers without model#421
Conversation
276889c to
7054dff
Compare
arromanoff
left a comment
There was a problem hiding this comment.
In general it now looks like snippy will dump initial registers with and without model via different mechanisms (in simulator and this new dump... function). Can we unify them instead?
| @@ -0,0 +1,20 @@ | |||
| # RUN: rm -f %t.initial-regs.yaml | |||
| # RUN: llvm-snippy %s -mtriple=riscv64-unknown-elf -num-instrs=10 \ | |||
There was a problem hiding this comment.
| # RUN: llvm-snippy %s -mtriple=riscv64-unknown-elf -num-instrs=10 \ | |
| # RUN: llvm-snippy %s -mtriple=riscv64 -num-instrs=10 \ |
There was a problem hiding this comment.
Also add -model-plugin None
| @@ -0,0 +1,20 @@ | |||
| # RUN: rm -f %t.initial-regs.yaml | |||
| sections: | ||
| - name: text | ||
| VMA: 0x1000 | ||
| LMA: 0x1000 | ||
| SIZE: 0x1000 | ||
| ACCESS: rx | ||
| - name: data | ||
| VMA: 0x3000 | ||
| LMA: 0x3000 | ||
| SIZE: 0x1000 | ||
| ACCESS: rw |
There was a problem hiding this comment.
Can we reuse sections from Inputs/ directory?
7054dff to
e9ab843
Compare
|
Updated the PR on top of the current main and addressed the review comments. Initial register dumping is now handled through SnippyProgramContext and reused from both model and no-model paths. The test was also updated to use Inputs/sections.yaml, remove the manual cleanup step, and explicitly pass --model-plugin=None. |
| const TargetSubtargetInfo &STI, | ||
| const RegPoolWrapper &RP) { | ||
| assert(!TargetContext && "Double context insertion"); | ||
| InitialStateSubtarget = &STI; |
There was a problem hiding this comment.
I don't like this new member and it's usage
We dump register state after we generated code so there're no problems with getting subtarget from LLVMState at this point. Just call getLLVMState().getSubtargetImpl
| ProgContext.dumpInitialRegisterState( | ||
| PassCfg.RegistersConfig.InitialStateOutputYaml); | ||
|
|
There was a problem hiding this comment.
There on the call site you can get the subtarget
auto *EntryFunc = MainModule.getFunction(ProgramCfg.EntryPointName);
assert(EntryFunc);
auto &ST = State.getSubtargetImpl(*EntryFunc);
// Now you can call
dumpInitialRegisterState(PassCfg.RegistersConfig.InitialStateOutputYaml, ST);
| const IRegisterState & | ||
| getInitialRegisterState(const TargetSubtargetInfo &ST) const; | ||
|
|
||
| void dumpInitialRegisterState(StringRef YamlPath) const; |
There was a problem hiding this comment.
Hence this overload is redundant
|
|
||
| void dumpInitialRegisterState(StringRef YamlPath) const; | ||
| void dumpInitialRegisterState(StringRef YamlPath, | ||
| const TargetSubtargetInfo &ST) const; |
There was a problem hiding this comment.
I'd also separate concerns here. Let's separate fetching this state and dumping it. So in FlowGenerator you can do:
auto &RegState = ProgramCtx.getInitialRegisterState(ST);
RegState.saveAsYAMLFile(PassCfg.RegistersConfig.InitialStateOutputYaml);
Fixes #12
Previously, initial register state was dumped only through the simulator path.
As a result, --dump-initial-registers-yaml did not work when model-plugin was
set to None, even though the initial register state was already available.
This change dumps the initial register state directly in the no-model path.
Added a regression test based on the reproducer from the issue.