Request for new pseudoinstruction: dec X / dec Y

[henrygab]

I have a recommendation for a new pseudoinstruction, and explanation on why it's so useful.

Is this the right depot to file the issue in?

If yes, then I will change the title of the issue to:

Request for new pseudoinstruction: dec X / dec Y

Assembler Syntax

dec <target>, where <target> is either X or Y

For target X:

• Assembly equivalent:

      jmp (x--), <uniquelabel>
  <uniquelabel>:

• 16-byte opcode: 0b_000_ddddd_010_aaaaa
  • ddddd represents the delay/side-set bits
  • aaaaa represents the encoded address to jump to

Edge cases

Normally, the dec <target> pseudoinstruction encodes to a jmp to the next instruction in the program.
Therefore, pioasm would need to handle the following two edges cases:

1. Where the pseudoinstruction precedes a .wrap directive -- encode the jmp address to the .wraptarget
2. Where the pseudoinstruction is the last instruction of the program -- encode the jmp address to the .wraptarget

Why do this?

Two reasons: First, to increase awareness of how to do this. Secondly, to avoid incorrectly written implementations.

When looking for how to modify the data in a register, a user will first look at the set and mov instructions. The impression given is that only negation and reversing the bits are available. However, decrement is a supported atomic operation!

There's also the problem of buggy attempts, where a person initially has an unconditional jmp, needs to decrement the register at the same time, and converts that to a jmp (x--) label ... thus introducing a bug, because if the register value pre-execution was zero, the jump will not happen. (yes, real world story)

[NollKollTroll]

This would have saved me some time today, trying to come up with a dec X/Y solution. Would be really handy to not be bothered with a unique label that can potentially be wrong.

[visenri]

I've also been thinking recently about a new pseudo instruction like this.
It could be handy, it makes the code more readable and less error prone.

[henrygab]

This fully backward-compatible change (only changes sources, no new emitted instructions) is still a feature worth adding....

[magy00]

I think it isn't as simple as it might look like. I think, currently, instructions and symbols use the same namespace. Any program that is using a symbol (or label) dec and was correct before would produce an error, unless the new instruction would, for example, only be enabled with a new directive or command line option.

So it would require careful changes in the grammar otherwise (see e. g. also #1951 and #2163/#2484). But that would be nice as well…

[henrygab]

I admit I'm having trouble visualizing a well-formed program that, with adding the dec {x | y} pseudo-instruction, would fail to compile. For example, I think a label requires :? You seem more experienced with grammar parsing ...

Could you provide simple example(s) that would cause trouble? (They'd also be useful for tests, since they'd be important edge cases.)

[magy00]

Let's say you had the following code already:

.program test
    jmp x--, dec
dec:
    nop

This currently works fine because dec isn't an instruction yet. To emulate that, replace both dec with a word that is already a keyword (let's say, next or jmp or X or manual), and you'll get an error.

[henrygab]

The current compiler might choose to emit an error for current keywords, when they are used as a label. I not yet seeing where this would be required behavior when parsing the .pio program?

---

At first glance, that appears to be a design choice, and not a hard requirement.

Here's what each line must parse as:

pico-sdk/tools/pioasm/parser.yy

Lines 176 to 185 in ee68c78

	line:
	PROGRAM ID { if (!pioasm.add_program(@$, $2)) { std::stringstream msg; msg << "program " << $2 << " already exists"; error(@$, msg.str()); abort(); } }
	| directive
	| instruction { pioasm.get_current_program(@1, "instruction").add_instruction($1); }
	| label_decl instruction { auto &p = pioasm.get_current_program(@2, "instruction"); p.add_label($1); p.add_instruction($2); }
	| label_decl { pioasm.get_current_program(@1, "label").add_label($1); }
	| code_block
	| %empty
	| error { if (pioasm.error_count > 6) { std::cerr << "\ntoo many errors; aborting.\n"; YYABORT; } }
	;

Of interest are lines 179-181, which allow a label_decl, a label_decl followed by an instruction, or just an instruction. The label_decl is defined to be any symbol_def, followed by COLON.

pico-sdk/tools/pioasm/parser.yy

Lines 191 to 192 in ee68c78

	label_decl:
	symbol_def COLON { $1->is_label = true; $$ = $1; }

Meanwhile, each potential instruction starts with a base_instruction:

pico-sdk/tools/pioasm/parser.yy

Lines 251 to 256 in ee68c78

	instruction:
	base_instruction sideset delay { $$ = $1; $$->sideset = $2; $$->delay = $3; }
	| base_instruction delay sideset { $$ = $1; $$->delay = $2; $$->sideset = $3; }
	| base_instruction sideset { $$ = $1; $$->sideset = $2; $$->delay = resolvable_int(@$, 0); }
	| base_instruction delay { $$ = $1; $$->delay = $2; }
	| base_instruction { $$ = $1; $$->delay = resolvable_int(@$, 0); }

And ... none of the valid base_instruction overlap with symbol_def because symbol_def requires a trailing colon:

pico-sdk/tools/pioasm/parser.yy

Lines 259 to 275 in ee68c78

	base_instruction:
	NOP { $$ = std::shared_ptr<instruction>(new instr_nop(@$)); }
	| JMP condition comma expression { $$ = std::shared_ptr<instruction>(new instr_jmp(@$, $2, $4)); }
	| WAIT value wait_source { $$ = std::shared_ptr<instruction>(new instr_wait(@$, $2, $3)); }
	| WAIT wait_source { $$ = std::shared_ptr<instruction>(new instr_wait(@$, resolvable_int(@$, 1), $2)); }
	| IN in_source comma value { $$ = std::shared_ptr<instruction>(new instr_in(@$, $2, $4)); }
	| OUT out_target comma value { $$ = std::shared_ptr<instruction>(new instr_out(@$, $2, $4)); }
	| PUSH if_full blocking { $$ = std::shared_ptr<instruction>(new instr_push(@$, $2, $3)); }
	| PULL if_empty blocking { $$ = std::shared_ptr<instruction>(new instr_pull(@$, $2, $3)); }
	| MOV mov_target comma mov_op mov_source { $$ = std::shared_ptr<instruction>(new instr_mov(@$, $2, $5, $4)); }
	| IRQ irq_modifiers value REL { $$ = std::shared_ptr<instruction>(new instr_irq(@$, $2, $3, 2)); }
	| IRQ PREV irq_modifiers value { pioasm.check_version(1, @$, "irq prev"); $$ = std::shared_ptr<instruction>(new instr_irq(@$, $3, $4, 1)); }
	| IRQ NEXT irq_modifiers value { pioasm.check_version(1, @$, "irq next"); $$ = std::shared_ptr<instruction>(new instr_irq(@$, $3, $4, 3)); }
	| IRQ PREV irq_modifiers value REL { pioasm.check_version(1, @$, "irq prev"); error(@5, "'rel' is not supported for 'irq prev'"); }
	| IRQ NEXT irq_modifiers value REL { pioasm.check_version(1, @$, "irq next"); error(@5, "'rel' is not supported for 'irq next'"); }
	| IRQ irq_modifiers value { $$ = std::shared_ptr<instruction>(new instr_irq(@$, $2, $3)); }
	| SET set_target comma value { $$ = std::shared_ptr<instruction>(new instr_set(@$, $2, $4)); }

And ... once a line is matching jmp, the expression would match ID (a deferred reference to a label).

---

Thus, the context of the current parsing makes it clear whether dec is a label or instruction. Put another way, it appears that, if the line starts with dec, whether that dec is a label or an instruction depends on whether it is followed by COLON (aka :).

---

Example using `dec` as both label and instruction

.program test2
    jmp x--, dec
dec:
    nop
    dec x
    nop

If I manually parse that, it seems to parse without error?

---

Am I missing a case where there would be ambiguity, or mandatory failure?

[henrygab]

Even where there is a namespace collision, this is not a compatibility problem or caused by a new instruction. The parser functions add_symbol() and add_label() appear to have the potential to conflict:

pico-sdk/tools/pioasm/parser.yy

Lines 194 to 195 in ee68c78

	directive:
	DEFINE symbol_def expression { $2->is_label = false; $2->value = $3; pioasm.get_current_program(@1, ".define", false, false).add_symbol($2); }

.program test3
.define dec 12
    jmp x--, dec
dec:
    nop
    dec x
    nop

But, the conflict is between the define and the label ... not the instruction?

Compare add_instruction() vs. add_symbol() / add_label()

pico-sdk/tools/pioasm/pio_assembler.cpp

Lines 38 to 53 in ee68c78

	void program::add_instruction(std::shared_ptr<instruction> inst) {
	uint limit = MAX_INSTRUCTIONS;
	if (instructions.size() >= limit) {
	// todo take offset into account
	std::stringstream msg;
	msg << "program instruction limit of " << limit << " instruction(s) exceeded";
	throw syntax_error(inst->location, msg.str());
	}
	if (!sideset_opt && !inst->sideset) {
	std::stringstream msg;
	msg << "instruction requires 'side' to specify side set value for the instruction because non optional sideset was specified for the program at " << sideset.location;
	throw syntax_error(inst->location, msg.str());
	}
	inst->pre_validate(*this);
	instructions.push_back(inst);
	}

pico-sdk/tools/pioasm/pio_types.h

Lines 349 to 352 in ee68c78

	void add_label(std::shared_ptr<symbol> label) {
	label->value = resolvable_int(label->location, instructions.size());
	add_symbol(label);
	}

pico-sdk/tools/pioasm/pio_assembler.cpp

Lines 81 to 97 in ee68c78

	void program::add_symbol(std::shared_ptr<symbol> symbol) {
	const auto &existing = pioasm->get_symbol(symbol->name, this);
	if (existing) {
	std::stringstream msg;
	if (symbol->is_label != existing->is_label) {
	msg << "'" << symbol->name << "' was already defined as a " << (existing->is_label ? "label" : "value")
	<< " at " << existing->location;
	} else if (symbol->is_label) {
	msg << "label '" << symbol->name << "' was already defined at " << existing->location;
	} else {
	msg << "'" << symbol->name << "' was already defined at " << existing->location;
	}
	throw syntax_error(symbol->location, msg.str());
	}
	symbols.insert(std::pair<std::string, std::shared_ptr<::symbol>>(symbol->name, symbol));
	ordered_symbols.push_back(symbol);
	}

[magy00]

My point is that it would be easier to do after some internal cleanup (separation of symbols, instructions, operands, directives; proper expressions; conditional assembly; etc.). Requests for pioasm have been queueing up for years (I guess it works passable enough and they don't have enough engineering time to justify them yet).

[kilograham]

note also that other things like CircuitPython support parsing the .pio format

[magy00]

I suspect it wouldn't be a problem on CircuitPython's pioasm, though (I think, e. g. next is allowed as a label there while on SDK 2's pioasm it isn't anymore even with .pio_version 0).