ume: Add interpolation to rv64ume and custom template tags#834
ume: Add interpolation to rv64ume and custom template tags#834arcane-quill wants to merge 18 commits into
Conversation
|
I think the branch of this PR isn't cleanly based on a fresh master, because it contains some of my commits that are already on master. So let's rebase on master with (on your branch): There might be some conflicts but if you execute these commands in IntelliJ you'll get some help resolving the conflicts in a nice UI. 😉 |
I tried to rebase twice now but it doesnt seem to be fixed @flofriday |
|
Yeah sometimes rebasing still contains some weired artifacts, but I just tried it on my machine and it (mostly) worked. I'll send you a DM (on matrix), let's figure it out there. |
ce9eaf4 to
0d64625
Compare
2c3616b to
4d6afb8
Compare
7d8ab0a to
4d6afb8
Compare
4d6afb8 to
f54e597
Compare
4d6afb8 to
f54e597
Compare
Jozott00
left a comment
There was a problem hiding this comment.
Unfortunately the current state is too RISC-V specific and needs an overhaul. Please take a closer look at other VIAM definitions and how they are handled in the code emission.
Thanks for the extensive review, I'm working on it now |
|
hi @Jozott00 I resolved the stuff that was missing, hopefully we can get this merged now and do u know why passorders keeps telling me there is a merge conflict despite none of it showing up locally, even after rebasing? |
Mostly guesses here but I assume you are not syncing your local branches with the remote (GitHub). For example git rebase masterIsn't good enough to rebase with the newest changes from GitHub. The long way is to do: git checkout master
git pull # (download from the remote, and also update the current master branch to the newest version from the remote)
git checkout <your-branch>
git rebase masterBut you can even shorten this a bit with git fetch # Downloads the branches from the remote but does not update the local branches only the branches prefixed with origin will be updated, so master is still stuck on an old commit but origin/master is synced with the remote
git rebase origin/master |
There was a problem hiding this comment.
I think there are still some important things to change. Mostly, what and how the VIAM definition stores specification details.
I also think, that we should reduce the scope for the PR a bit, as signals are just a bit too much to start with. Therefore, let's completely remove all logic that is related to signals.
The UME generation requires quite a bit of information from the ABI definition and could share some constructs (like alias registers and special register references). Therefore, I will do a little ABI refactoring, so you can use RegisterRef instead of defining you own RegisterLocation as mentioned in a comment below.
EDIT: Also note, that this PR won't fully close #761 as there is still RISC-V specific stuff in the template files.
|
|
||
| private final Identifier excCauseVar; | ||
| private final RegisterTensor mainRegisterFile; | ||
| private final RegisterUtils.Register sysReg; |
There was a problem hiding this comment.
major: Using vadl.lcb.templateUtils.RegisterUtils.Register is the wrong abstraction
First, it is part of the LCB and should not be referenced by the vadl.viam package, second, it is a rendering utility, not a VIAM construct. It does not contain the original RegisterTensor and has exactly one index.
Instead, add a new class in the vadl.viam package:
public record RegisterLocation(
RegisterResource resource,
List<Constant.Value> indices,
SourceLocation location
) implements WithLocation {
}
And use it instead. This will model those register references/locations much better and remains generic.
| private final ExceptionDef breakpointExcName; | ||
| private final ExceptionDef illegalInstrExcName; | ||
| private final Identifier initialPc; | ||
| private final Identifier initialSp; |
There was a problem hiding this comment.
question: What is the initialPC and initialSp? Can you give me an example, cause I am confused how its value can be represented as Identifier.
From what I understand, this can be removed, as it can be synthesized later by the generator.
| if (args == null || args.isEmpty()) { | ||
| throw new IllegalArgumentException("args must not be null/empty"); | ||
| } | ||
|
|
||
| if (excIds == null || excIds.isEmpty()) { | ||
| throw new IllegalArgumentException("excIds must not be null/empty"); | ||
| } |
There was a problem hiding this comment.
minor: We use nullaway, so null checking of arguments that are not annoated with @Nullable isn't necessary.
| if (args == null || args.isEmpty()) { | ||
| throw new IllegalArgumentException("args must not be null/empty"); | ||
| } | ||
|
|
||
| if (excIds == null || excIds.isEmpty()) { | ||
| throw new IllegalArgumentException("excIds must not be null/empty"); | ||
| } |
There was a problem hiding this comment.
question: Is there a reason why you require the args to be non-empty? From the generator side, it shouldn't matter if there is any argument.
| private final Map<String, Integer> excIds; | ||
| private final Instruction syscallInstr; | ||
| private final ExceptionDef syscallException; | ||
| private final ExceptionDef breakpointExcName; |
There was a problem hiding this comment.
minor: Should be breakpointExc instead of breakpointExcName
(same below)
| [# th:if="${config.hasIcacheFlush}"] | ||
| if (env->[(${config.mainRegisterFile})][ [(${config.sysReg})] ] == TARGET_NR_[(${gen_arch_lower})]_flush_icache) { | ||
| /* no-op in QEMU; TB invalidation is automatic */ | ||
| ret = 0; | ||
| } else { | ||
| [/] | ||
| ret = do_syscall(env, | ||
| env->x[RV64UME_REG_A7], | ||
| env->x[RV64UME_REG_A0], | ||
| env->x[RV64UME_REG_A1], | ||
| env->x[RV64UME_REG_A2], | ||
| env->x[RV64UME_REG_A3], | ||
| env->x[RV64UME_REG_A4], | ||
| env->x[RV64UME_REG_A5], | ||
| 0, 0); | ||
| env->[(${config.mainRegisterFile})][ [(${config.sysReg})] ], | ||
| [# th:each="arg : ${config.args}"] | ||
| env->[(${config.mainRegisterFile})][ [(${arg})] ], | ||
| [/] | ||
| 0, 0); | ||
| [# th:if="${config.hasIcacheFlush}"] | ||
| } | ||
| [/] |
There was a problem hiding this comment.
major: Just assume there is no icache-flush
| exit(EXIT_FAILURE); | ||
| } | ||
| [/] | ||
| [# th:if="${#strings.isEmpty(config.excCauseVar)}"] |
There was a problem hiding this comment.
major: This check is unnecessary, as exceptions in the ISS are always rendered as cause variables in the state.
| env->pc = regs->sepc; | ||
| env->x[RV64UME_REG_SP] = regs->sp; | ||
| env->[(${pc_reg.name_lower})] = regs->[(${config.initialPc})]; | ||
| env->[(${config.mainRegisterFile})][ [(${config.spReg})] ] = regs->[(${config.initalSp})]; |
There was a problem hiding this comment.
major: As we just hardcode the target_pt_regs anyway, just use regs->pc and regs->sp instead.
There was a problem hiding this comment.
minor: This whole table is currently hardcoded. However, I think we should do the dynamic interpolation in a separate PR.
| if (inputPath != null && inputPath.getFileName().endsWith("rv64ume.vadl")) { | ||
| order | ||
| .add(new UmeHardcodedRiscvDefinitionPass(config)) | ||
| .add(new UmeTemplateRenderingPass(config, "cpu_loop.c")) |
There was a problem hiding this comment.
major: The cpu_loop.c is now emitted twice
…` interface (#948) This PR extracts most of the `Abi.RegisterRef` to `vadl.viam.RegisterRef`, so the datastructure can be reused in other definitions like the UME (#834). Additionally, it removes the `GeneratesRegisterFileName` which does not fit into the VIAM hierarchy and is already covered by the `RegisterResource` class.
PR for issue #757 (and #761, #759)
interpolated hardcoded rv64ume files from linux-user
created UmeHardcodedRiscvDefinitionPass and UmeTemplateRenderingPass to add custom template tags (e.g. registers, exceptions..) with hardcoded values that later get filled in by the vadl file
added VIAM definition UserModeEmulation