bitwalker
commented
2 weeks ago One other potential change: in the assembler we use nop while in the VM we use Noop - should we use Noop in the assembler for consistency? Not a strong preference from my side - so, if you prefer keeping nop in the assembler, that's fine too.
I noticed this too, albeit after I had mostly finished implementation. My rationale for leaving it this way in the surface language is because nop is the mnemonic used in all other assembly languages I'm aware of that have such an instruction, so you don't have to try and rewire any muscle memory for it in MASM. That said, I don't think it will be super common to write nop by hand (much like traditional asm), so it doesn't matter that much. All else being equal I'd actually be inclined to rename Noop to Nop rather than the inverse, but since this is new code, I don't have a problem with sticking to Noop everywhere if that's the preference.
Regarding if.false - I think that's a good idea. I'm not sure how much work that would involve though. If it is just a couple dozen lines of code - probably fine to include in this PR, but if it that's a bigger change, I'd rather do it in a different PR.
It's a simple change actually, the grammar rule in the parser is already written. I'll go ahead and add a commit with that, and let you make the judgement call on whether to split it out or not.
Let's update the changelog.
👍
Should we update the docs to list nop as a valid instruction?
May as well, I don't expect it to be used in general, but there is little sense in not documenting it, so I'll go ahead and do that.
bobbinth
@Al-Kindi-0 I accidentally added you as a reviewer on this, and apparently that can't be undone for whatever reason, so feel free to ignore, or not, your call!
As mentioned in the title, this PR does two main things:
nopin Miden Assembly source files, corresponding to the nop instruction in the VMif.truecan be empty when parsed, but not both, and are canonicalized as a block containing a singlenopinstruction. This is handled both when parsing source files, and pretty printing blocks.The motivation behind this comes down to a few things:
push.0 drop) in the "unused" branch. Allowing the use ofnopin these cases is cleaner.nopfor empty blocks when compiling to MAST, so allowing this implicit behavior to be made explicit can have the benefit of less "magic" happening in the assembler. We could also use this to simplify the assembler a bit, by requiring that at the time of conversion to MAST, that all blocks contain at least one instruction.nopto be elided for convenience, and canonicalize it in the parser. The semantics are identical, and there is little reason to have the parser fail on an empty block when it represents a perfectly valid program.The compiler has already hit this issue, and solving it there is awkward in comparison to doing so in the assembler, where such logic belongs anyway IMO. Emitting an instruction to invert the condition so that the shorthand
if.true .. endsyntax can be used causes false-conditioned branches to be more expensive than true-conditioned branches (by 1 cycle, which is admittedly tiny, but nevertheless, it makes no sense to penalize conditional branching this way).As an aside, as we have explicit one-sided conditional branches already, e.g.
if.true ... end, it might be desirable to add syntax, e.g.if.false .. endorunless.true .. endfor the false-conditoned case. I did not do that here, and instead simply allow one to writeif.true else .. endas a synonym for theif.false .. endsyntax. Let me know your thoughts here.I want to note that these changes are not designed for hand-written MASM code - I don't consider either of these the sort of features that need to be advertised, but are instead accommodations specifically oriented towards compilers. When hand-writing MASM, you aren't going to use either of these features in practice. However, it is important that code emitted by compilers be able to be emitted in text form, and properly parsed, and these tweaks will make that much more straightforward.