gh-raspberrypi/pico-sdk

Watch

Fork 0

pioasm: allow optional target for .wrap #2163

New issue

Open

opened 2025-01-05 21:40:47 -06:00 by magy00 · 6 comments

magy00 commented

2025-01-05 21:40:47 -06:00

(Migrated from github.com)

Update: see also proposed doc changes in #2484

It could sometimes be convenient (and possibly allow more descriptive source code) if .wrap would optionally accept the same target expressions as jmp (with .wrap_target as the default):

.wrap (<target>)

E.g.

loop:
    mov osr, rxfifo[y]
    ...
    jmp y--, loop
    .wrap loop

would be equivalent to

.wrap_target
loop:
    mov osr, rxfifo[y]
    ...
    jmp y--, loop
    .wrap

**Update:** see also proposed doc changes in #2484 It could sometimes be convenient (and possibly allow more descriptive source code) if `.wrap` would optionally accept the same target expressions as `jmp` (with `.wrap_target` as the default): .wrap (<target>) E.g. ``` loop: mov osr, rxfifo[y] ... jmp y--, loop .wrap loop ``` would be equivalent to ``` .wrap_target loop: mov osr, rxfifo[y] ... jmp y--, loop .wrap ```

magy00 commented

2025-01-13 03:24:05 -06:00

(Migrated from github.com)

Perhaps even allow .wrap <target> at the end of an instruction (like side).

It would also be convenient to have e.g. . return the current location. Then

delay:
    jmp x--, delay

could be written as

    jmp x--, .

and

    .origin 0
    irq set   0
.wrap
    irq clear 0
.wrap_target
    …
    out pc, 1

    .origin 0
    irq set   0   .wrap skip
    irq clear 0
skip:
    …
    out pc, 1

or as an (extreme) example (alternative ringing cadence generator):

.program ring_sequence_irq_0
    .pio_version 1
    .fifo txget
    //mov y, ~null    // start of reversed sequence

    .origin   0
    irq set   0   .wrap .+1
    irq clear 0

    mov osr, rxfifo[y]
    mov x, osr
    jmp x--, .      // delay

    mov osr, y      // used to toggle irq flag
    jmp y--, .+1    // decrement only

    out pc, 1

Perhaps even allow `.wrap <target>` at the end of an instruction (like `side`). It would also be convenient to have e.g. `.` return the current location. Then ``` delay: jmp x--, delay ``` could be written as ``` jmp x--, . ``` and ``` .origin 0 irq set 0 .wrap irq clear 0 .wrap_target … out pc, 1 ``` as ``` .origin 0 irq set 0 .wrap skip irq clear 0 skip: … out pc, 1 ``` or as an (extreme) example (alternative [ringing cadence generator](https://forums.raspberrypi.com/viewtopic.php?p=2284726#p2284726)): ``` .program ring_sequence_irq_0 .pio_version 1 .fifo txget //mov y, ~null // start of reversed sequence .origin 0 irq set 0 .wrap .+1 irq clear 0 mov osr, rxfifo[y] mov x, osr jmp x--, . // delay mov osr, y // used to toggle irq flag jmp y--, .+1 // decrement only out pc, 1 ```

magy00 commented

2025-01-22 22:57:58 -06:00

(Migrated from github.com)

Maybe moving WORD value from directive: to base_instruction: in parser.yy could also be convenient (the .word directive basically is an instruction). Could be useful for things like:

.define public  UI  5  // unit interval (4-32), in integer .clock_div cycles
.define public  PREAMBLE_B_Z    0b00010111  // sub-frame 1 at start of block

    // assemble either mov pins, null or mov pins, ~null
    .define mov_pins_null 0b1010000000000000
    .word (mov_pins_null + (PREAMBLE_B_Z>>4 << 3))    [UI-1]
    .word (mov_pins_null + (PREAMBLE_B_Z>>5 << 3))    [UI-1]
    .word (mov_pins_null + (PREAMBLE_B_Z>>6 << 3))    [UI-1]

Maybe moving `WORD value` from `directive:` to `base_instruction:` in `parser.yy` could also be convenient (the `.word` directive basically _is_ an instruction). Could be useful for things like: ``` .define public UI 5 // unit interval (4-32), in integer .clock_div cycles .define public PREAMBLE_B_Z 0b00010111 // sub-frame 1 at start of block // assemble either mov pins, null or mov pins, ~null .define mov_pins_null 0b1010000000000000 .word (mov_pins_null + (PREAMBLE_B_Z>>4 << 3)) [UI-1] .word (mov_pins_null + (PREAMBLE_B_Z>>5 << 3)) [UI-1] .word (mov_pins_null + (PREAMBLE_B_Z>>6 << 3)) [UI-1] ```

magy00 commented

2025-03-12 05:50:41 -05:00

(Migrated from github.com)

Suggestion for above (except for allowing .wrap at the end of an instruction):

diff --git tools/pioasm/lexer.ll tools/pioasm/lexer.ll
index fdf8c72..2b76202 100644
--- tools/pioasm/lexer.ll
+++ tools/pioasm/lexer.ll
@@ -220,6 +220,8 @@ output_fmt    [^%\n]+
 
 {comment}                           { }
 
+"."                                 return yy::parser::make_DOT(loc);
+
 .                                   { throw yy::parser::syntax_error(loc, "invalid character: " + std::string(yytext)); }
 
 %%
diff --git tools/pioasm/parser.yy tools/pioasm/parser.yy
index 730bc92..910a6a9 100644
--- tools/pioasm/parser.yy
+++ tools/pioasm/parser.yy
@@ -85,6 +85,7 @@
     DOT_SET         ".set"
     DOT_OUT         ".out"
     DOT_IN          ".in"
+    DOT             "."
 
     JMP             "jmp"
     WAIT            "wait"
@@ -203,8 +204,8 @@ directive:
   | DOT_OUT value direction autop threshold { pioasm.get_current_program(@1, ".out", true).set_out(@$, $2, $3, $4, $5); }
   | DOT_SET value                     { pioasm.get_current_program(@1, ".set", true).set_set_count(@$, $2); }
   | WRAP_TARGET                       { pioasm.get_current_program(@1, ".wrap_target").set_wrap_target(@$); }
+  | WRAP expression                   { pioasm.get_current_program(@1, ".wrap").set_wrap(@$, $2); }
   | WRAP                              { pioasm.get_current_program(@1, ".wrap").set_wrap(@$); }
-  | WORD value                        { pioasm.get_current_program(@1, "instruction").add_instruction(std::shared_ptr<instruction>(new instr_word(@$, $2))); }
   | LANG_OPT NON_WS NON_WS ASSIGN INT  { pioasm.get_current_program(@1, ".lang_opt").add_lang_opt($2, $3, std::to_string($5)); }
   | LANG_OPT NON_WS NON_WS ASSIGN STRING { pioasm.get_current_program(@1, ".lang_opt").add_lang_opt($2, $3, $5); }
   | LANG_OPT NON_WS NON_WS ASSIGN NON_WS { pioasm.get_current_program(@1, ".lang_opt").add_lang_opt($2, $3, $5); }
@@ -224,6 +225,7 @@ directive:
 /* value is a more limited top level expression... requiring parenthesis */
 %type <std::shared_ptr<resolvable>> value;
 value: INT { $$ = resolvable_int(@$, $1); }
+     | DOT { $$ = resolvable_int(@$, pioasm.get_current_program(@1, ".").instructions.size()); }
      | ID { $$ = std::shared_ptr<resolvable>(new name_ref(@$, $1)); }
      | LPAREN expression RPAREN { $$ = $2; }
 
@@ -257,7 +259,8 @@ instruction:
 
 %type <std::shared_ptr<instruction>> base_instruction;
 base_instruction:
-    NOP                                                   { $$ = std::shared_ptr<instruction>(new instr_nop(@$)); }
+    WORD value                                            { $$ = std::shared_ptr<instruction>(new instr_word(@$, $2)); }
+    | 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)); }
diff --git tools/pioasm/pio_assembler.cpp tools/pioasm/pio_assembler.cpp
index a36854b..861d2f5 100644
--- tools/pioasm/pio_assembler.cpp
+++ tools/pioasm/pio_assembler.cpp
@@ -161,6 +161,17 @@ void program::set_wrap(const yy::location &l) {
     wrap = resolvable_int(l, instructions.size() - 1);
 }
 
+void program::set_wrap(const yy::location &l, rvalue target) {
+    set_wrap(l);
+
+    if (wrap_target) {
+        std::stringstream msg;
+        msg << ".wrap_target was already specified at " << wrap_target->location;
+        throw syntax_error(l, msg.str());
+    }
+    wrap_target = std::move(target);
+}
+
 void program::set_wrap_target(const yy::location &l) {
     if (wrap_target) {
         std::stringstream msg;
@@ -299,12 +310,12 @@ raw_encoding instruction::raw_encode(program& program) {
     throw syntax_error(location, "internal error");
 }
 
-uint instr_word::encode(program &program) {
+raw_encoding instr_word::raw_encode(program& program) {
     uint value = encoding->resolve(program);
     if (value > 0xffffu) {
         throw syntax_error(location, ".word value must be a positive 16 bit value");
     }
-    return value;
+    return {inst_type(0), value >> 5, value & 0x1fu};
 }
 
 uint instr_mov::get_push_get_index(const program &program, extended_mov index) {
diff --git tools/pioasm/pio_types.h tools/pioasm/pio_types.h
index 9f9fb0e..532d918 100644
--- tools/pioasm/pio_types.h
+++ tools/pioasm/pio_types.h
@@ -320,6 +320,8 @@ struct program : public src_item {
 
     void set_wrap(const yy::location &l);
 
+    void set_wrap(const yy::location &l, rvalue target);
+
     void set_sideset(const yy::location &l, rvalue _sideset, bool optional, bool pindirs) {
         sideset = rvalue_loc(_sideset, l);
         sideset_opt = optional;
@@ -485,7 +487,7 @@ struct instr_word : public instruction {
 
     instr_word(const yy::location &l, rvalue encoding) : instruction(l), encoding(std::move(encoding)) {}
 
-    uint encode(program &program) override;
+    raw_encoding raw_encode(program &program) override;
 };
 
 #endif
diff --git tools/pioasm/test/amethyst.pio tools/pioasm/test/amethyst.pio
index 6b81604..eb02933 100644
--- tools/pioasm/test/amethyst.pio
+++ tools/pioasm/test/amethyst.pio
@@ -45,6 +45,16 @@ wait gpio 40
 .mov_status txfifo < 12
 .mov_status irq next set 3

+.program wrap
+decrement:
+    jmp x--, .+1
+skip:
+    .wrap .+1
+delay:
+    jmp x--, .
+jump:
+    .word (.-1) [2]
+
 .program python
 .pio_version 1
 wait 0 jmppin

Suggestion for above (except for allowing `.wrap` at the end of an instruction): ```diff diff --git tools/pioasm/lexer.ll tools/pioasm/lexer.ll index fdf8c72..2b76202 100644 --- tools/pioasm/lexer.ll +++ tools/pioasm/lexer.ll @@ -220,6 +220,8 @@ output_fmt [^%\n]+ {comment} { } +"." return yy::parser::make_DOT(loc); + . { throw yy::parser::syntax_error(loc, "invalid character: " + std::string(yytext)); } %% diff --git tools/pioasm/parser.yy tools/pioasm/parser.yy index 730bc92..910a6a9 100644 --- tools/pioasm/parser.yy +++ tools/pioasm/parser.yy @@ -85,6 +85,7 @@ DOT_SET ".set" DOT_OUT ".out" DOT_IN ".in" + DOT "." JMP "jmp" WAIT "wait" @@ -203,8 +204,8 @@ directive: | DOT_OUT value direction autop threshold { pioasm.get_current_program(@1, ".out", true).set_out(@$, $2, $3, $4, $5); } | DOT_SET value { pioasm.get_current_program(@1, ".set", true).set_set_count(@$, $2); } | WRAP_TARGET { pioasm.get_current_program(@1, ".wrap_target").set_wrap_target(@$); } + | WRAP expression { pioasm.get_current_program(@1, ".wrap").set_wrap(@$, $2); } | WRAP { pioasm.get_current_program(@1, ".wrap").set_wrap(@$); } - | WORD value { pioasm.get_current_program(@1, "instruction").add_instruction(std::shared_ptr<instruction>(new instr_word(@$, $2))); } | LANG_OPT NON_WS NON_WS ASSIGN INT { pioasm.get_current_program(@1, ".lang_opt").add_lang_opt($2, $3, std::to_string($5)); } | LANG_OPT NON_WS NON_WS ASSIGN STRING { pioasm.get_current_program(@1, ".lang_opt").add_lang_opt($2, $3, $5); } | LANG_OPT NON_WS NON_WS ASSIGN NON_WS { pioasm.get_current_program(@1, ".lang_opt").add_lang_opt($2, $3, $5); } @@ -224,6 +225,7 @@ directive: /* value is a more limited top level expression... requiring parenthesis */ %type <std::shared_ptr<resolvable>> value; value: INT { $$ = resolvable_int(@$, $1); } + | DOT { $$ = resolvable_int(@$, pioasm.get_current_program(@1, ".").instructions.size()); } | ID { $$ = std::shared_ptr<resolvable>(new name_ref(@$, $1)); } | LPAREN expression RPAREN { $$ = $2; } @@ -257,7 +259,8 @@ instruction: %type <std::shared_ptr<instruction>> base_instruction; base_instruction: - NOP { $$ = std::shared_ptr<instruction>(new instr_nop(@$)); } + WORD value { $$ = std::shared_ptr<instruction>(new instr_word(@$, $2)); } + | 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)); } diff --git tools/pioasm/pio_assembler.cpp tools/pioasm/pio_assembler.cpp index a36854b..861d2f5 100644 --- tools/pioasm/pio_assembler.cpp +++ tools/pioasm/pio_assembler.cpp @@ -161,6 +161,17 @@ void program::set_wrap(const yy::location &l) { wrap = resolvable_int(l, instructions.size() - 1); } +void program::set_wrap(const yy::location &l, rvalue target) { + set_wrap(l); + + if (wrap_target) { + std::stringstream msg; + msg << ".wrap_target was already specified at " << wrap_target->location; + throw syntax_error(l, msg.str()); + } + wrap_target = std::move(target); +} + void program::set_wrap_target(const yy::location &l) { if (wrap_target) { std::stringstream msg; @@ -299,12 +310,12 @@ raw_encoding instruction::raw_encode(program& program) { throw syntax_error(location, "internal error"); } -uint instr_word::encode(program &program) { +raw_encoding instr_word::raw_encode(program& program) { uint value = encoding->resolve(program); if (value > 0xffffu) { throw syntax_error(location, ".word value must be a positive 16 bit value"); } - return value; + return {inst_type(0), value >> 5, value & 0x1fu}; } uint instr_mov::get_push_get_index(const program &program, extended_mov index) { diff --git tools/pioasm/pio_types.h tools/pioasm/pio_types.h index 9f9fb0e..532d918 100644 --- tools/pioasm/pio_types.h +++ tools/pioasm/pio_types.h @@ -320,6 +320,8 @@ struct program : public src_item { void set_wrap(const yy::location &l); + void set_wrap(const yy::location &l, rvalue target); + void set_sideset(const yy::location &l, rvalue _sideset, bool optional, bool pindirs) { sideset = rvalue_loc(_sideset, l); sideset_opt = optional; @@ -485,7 +487,7 @@ struct instr_word : public instruction { instr_word(const yy::location &l, rvalue encoding) : instruction(l), encoding(std::move(encoding)) {} - uint encode(program &program) override; + raw_encoding raw_encode(program &program) override; }; #endif diff --git tools/pioasm/test/amethyst.pio tools/pioasm/test/amethyst.pio index 6b81604..eb02933 100644 --- tools/pioasm/test/amethyst.pio +++ tools/pioasm/test/amethyst.pio @@ -45,6 +45,16 @@ wait gpio 40 .mov_status txfifo < 12 .mov_status irq next set 3 +.program wrap +decrement: + jmp x--, .+1 +skip: + .wrap .+1 +delay: + jmp x--, . +jump: + .word (.-1) [2] + .program python .pio_version 1 wait 0 jmppin ```

visenri commented

2025-03-14 16:33:41 -05:00

(Migrated from github.com)

I really like the idea of the optional target for ".wrap", but I think adding it to the end of an instruction with "side" and "delay" is too much:

.wrap_target
    out pins, 1 side 0 [1]
    pull ifempty side 1 [1]
.wrap

Could be written as:

loop:
    out pins, 1 side 0 [1]
    pull ifempty side 1 [1] .wrap loop

But I really find this one cleaner:

loop:
    out pins, 1 side 0 [1]
    pull ifempty side 1 [1] 
.wrap loop

Having labels in ".wrap" directives allows for:

Better name for the ".wrap_target" (label name).
Reuse of a label as ".wrap_target", otherwise, sometimes, you end up with a ".wrap_target" and a "label" to jump to the same instruction.

I'm also all in for the "current location" syntax, it avoids unneeded labels for the typical delays.
But I really prefer the "$" symbol instead of the ".", it is way more popular and makes it more "visible":

    jmp x--, $     // Delay loop.

Even better if expressions can be used:

    jmp y--, $+1    // Decrement only.

I will soon publish a VSCode extension for PIO ASM syntax (including new RP2350 syntax).
I will add support for these features if they are finally accepted.

I really like the idea of the optional target for ".wrap", but I think adding it to the end of an instruction with "side" and "delay" is too much: ```assembly .wrap_target out pins, 1 side 0 [1] pull ifempty side 1 [1] .wrap ``` Could be written as: ```assembly loop: out pins, 1 side 0 [1] pull ifempty side 1 [1] .wrap loop ``` But I really find this one cleaner: ```assembly loop: out pins, 1 side 0 [1] pull ifempty side 1 [1] .wrap loop ``` Having labels in ".wrap" directives allows for: - Better name for the ".wrap_target" (label name). - Reuse of a label as ".wrap_target", otherwise, sometimes, you end up with a ".wrap_target" and a "label" to jump to the same instruction. I'm also all in for the "current location" syntax, it avoids unneeded labels for the typical delays. But I really prefer the "$" symbol instead of the ".", it is way more popular and makes it more "visible": ```assembly jmp x--, $ // Delay loop. ``` Even better if expressions can be used: ```assembly jmp y--, $+1 // Decrement only. ``` I will soon publish a VSCode extension for PIO ASM syntax (including new RP2350 syntax). I will add support for these features if they are finally accepted.

magy00 commented

2025-03-15 06:54:19 -05:00

(Migrated from github.com)

But I really prefer the "$" symbol instead of the ".", it is way more popular and makes it more "visible"

That really depends on where you're coming from (Unix, PC, minicomputer, home computer, etc.).

gas uses . (see e. g. .macro case), and $ is sometimes either allowed in (local) symbols/labels or for starting comments:
The GNU Assembler: The Special Dot Symbol

Some others use * instead…

adding it to the end of an instruction

It's not a small change in the parser; that's why it's omitted in above patch (but it could more closely reflect how it's actually working: it's applied after an instruction is executed; not as an own instruction or before an instruction is executed).

Another relatively simple change would be to have an optional second argument to .wrap, like .wrap WRAP_BOTTOM WRAP_TOP, which could be placed anywhere in the program and refers to 2 labels (or expressions); but I'm not sure how useful it would actually be (except for explaining it in the docs; but more to type as well).

Well, how about having it optionally return an expression (I'm not really serious here):

loop:
    mov osr, rxfifo[y]
    ...
    jmp y--, .wrap loop

> But I really prefer the "$" symbol instead of the ".", it is way more popular and makes it more "visible" That really depends on where you're coming from (Unix, PC, minicomputer, home computer, etc.). `gas` uses `.` (see e. g. [`.macro case`](https://web.archive.org/web/20161118025739/ec.gnost.info/ec-18s/split/1YHT29WW.html#case)), and `$` is sometimes either allowed in (local) symbols/labels or for starting comments: [The GNU Assembler: The Special Dot Symbol](https://ftp.gnu.org/old-gnu/Manuals/gas/html_chapter/as_toc.html#TOC49) Some [others use `*`](https://web.archive.org/web/20190721204045/markus.gyger.org/1581/speed1581.lst.html) instead… > adding it to the end of an instruction It's not a small change in the parser; that's why it's omitted in above patch (but it could more closely reflect how it's actually working: it's applied _after_ an instruction is executed; not as an own instruction or before an instruction is executed). Another relatively simple change would be to have an optional second argument to `.wrap`, like `.wrap WRAP_BOTTOM WRAP_TOP`, which could be placed anywhere in the program and refers to 2 labels (or expressions); but I'm not sure how useful it would actually be (except for explaining it in the docs; but more to type as well). Well, how about having it optionally return an `expression` (I'm not really serious here): ``` loop: mov osr, rxfifo[y] ... jmp y--, .wrap loop ```

magy00 commented

2026-08-13 01:47:47 -05:00

(Migrated from github.com)