This is the sixth and final patch in a series of patches to the parser that
aims to reduce code size by compressing the data corresponding to the rules
of the grammar.
Prior to this set of patches the rules were stored as rule_t structs with
rule_id, act and arg members. And then there was a big table of pointers
which allowed to lookup the address of a rule_t struct given the id of that
rule.
The changes that have been made are:
- Breaking up of the rule_t struct into individual components, with each
component in a separate array.
- Removal of the rule_id part of the struct because it's not needed.
- Put all the rule arg data in a big array.
- Change the table of pointers to rules to a table of offsets within the
array of rule arg data.
The last point is what is done in this patch here and brings about the
biggest decreases in code size, because an array of pointers is now an
array of bytes.
Code size changes for the six patches combined is:
bare-arm: -644
minimal x86: -1856
unix x64: -5408
unix nanbox: -2080
stm32: -720
esp8266: -812
cc3200: -712
For the change in parser performance: it was measured on pyboard that these
six patches combined gave an increase in script parse time of about 0.4%.
This is due to the slightly more complicated way of looking up the data for
a rule (since the 9th bit of the offset into the rule arg data table is
calculated with an if statement). This is an acceptable increase in parse
time considering that parsing is only done once per script (if compiled on
the target).
Instead of each rule being stored in ROM as a struct with rule_id, act and
arg, the act and arg parts are now in separate arrays and the rule_id part
is removed because it's not needed. This reduces code size, by roughly one
byte per grammar rule, around 150 bytes.
The rule name is only used for debugging, and this patch makes things a bit
cleaner by completely separating out the rule name from the rest of the
rule data.
Each NLR implementation (Thumb, x86, x64, xtensa, setjmp) duplicates a lot
of the NLR code, specifically that dealing with pushing and popping the NLR
pointer to maintain the linked-list of NLR buffers. This patch factors all
of that code out of the specific implementations into generic functions in
nlr.c, along with a helper macro in nlr.h. This eliminates duplicated
code.
If MICROPY_NLR_SETJMP is not enabled and the machine is auto-detected then
nlr.h now defines some convenience macros for the individual NLR
implementations to use (eg MICROPY_NLR_THUMB). This keeps nlr.h and the
implementation in sync, and also makes the nlr_buf_t struct easier to read.
A function with a naked attribute must only contain basic inline asm
statements and no C code.
For nlr_push this means removing the "return 0" statement. But for some
gcc versions this induces a compiler warning so the __builtin_unreachable()
line needs to be added.
For nlr_jump, this function contains a combination of C code and inline asm
so cannot be naked.
This reverts commit 6a3a742a6c.
The above commit has number of faults starting from the motivation down
to the actual implementation.
1. Faulty implementation.
The original code contained functions like:
NORETURN void nlr_jump(void *val) {
nlr_buf_t **top_ptr = &MP_STATE_THREAD(nlr_top);
nlr_buf_t *top = *top_ptr;
...
__asm volatile (
"mov %0, %%edx \n" // %edx points to nlr_buf
"mov 28(%%edx), %%esi \n" // load saved %esi
"mov 24(%%edx), %%edi \n" // load saved %edi
"mov 20(%%edx), %%ebx \n" // load saved %ebx
"mov 16(%%edx), %%esp \n" // load saved %esp
"mov 12(%%edx), %%ebp \n" // load saved %ebp
"mov 8(%%edx), %%eax \n" // load saved %eip
"mov %%eax, (%%esp) \n" // store saved %eip to stack
"xor %%eax, %%eax \n" // clear return register
"inc %%al \n" // increase to make 1, non-local return
"ret \n" // return
: // output operands
: "r"(top) // input operands
: // clobbered registers
);
}
Which clearly stated that C-level variable should be a parameter of the
assembly, whcih then moved it into correct register.
Whereas now it's:
NORETURN void nlr_jump_tail(nlr_buf_t *top) {
(void)top;
__asm volatile (
"mov 28(%edx), %esi \n" // load saved %esi
"mov 24(%edx), %edi \n" // load saved %edi
"mov 20(%edx), %ebx \n" // load saved %ebx
"mov 16(%edx), %esp \n" // load saved %esp
"mov 12(%edx), %ebp \n" // load saved %ebp
"mov 8(%edx), %eax \n" // load saved %eip
"mov %eax, (%esp) \n" // store saved %eip to stack
"xor %eax, %eax \n" // clear return register
"inc %al \n" // increase to make 1, non-local return
"ret \n" // return
);
for (;;); // needed to silence compiler warning
}
Which just tries to perform operations on a completely random register (edx
in this case). The outcome is the expected: saving the pure random luck of
the compiler putting the right value in the random register above, there's
a crash.
2. Non-critical assessment.
The original commit message says "There is a small overhead introduced
(typically 1 machine instruction)". That machine instruction is a call
if a compiler doesn't perform tail optimization (happens regularly), and
it's 1 instruction only with the broken code shown above, fixing it
requires adding more. With inefficiencies already presented in the NLR
code, the overhead becomes "considerable" (several times more than 1%),
not "small".
The commit message also says "This eliminates duplicated code.". An
obvious way to eliminate duplication would be to factor out common code
to macros, not introduce overhead and breakage like above.
3. Faulty motivation.
All this started with a report of warnings/errors happening for a niche
compiler. It could have been solved in one the direct ways: a) fixing it
just for affected compiler(s); b) rewriting it in proper assembly (like
it was before BTW); c) by not doing anything at all, MICROPY_NLR_SETJMP
exists exactly to address minor-impact cases like thar (where a) or b) are
not applicable). Instead, a backwards "solution" was put forward, leading
to all the issues above.
The best action thus appears to be revert and rework, not trying to work
around what went haywire in the first place.
These were copied from the stm32 port (then stmhal) at the very beginning
of this port, with the anticipation that the esp8266 port would have board
definition files with a list of valid pins and their names. But that has
not been implemented and likely won't be, so remove the corresponding lines
from the Makefile.
This patch adds in internal config value MICROPY_HW_ENABLE_HW_I2C that is
automatically configured, and enabled only if one or more hardware I2C
ports are defined in the mpconfigboard.h file. If none are defined then
the pyb.I2C class is excluded from the build, along with all supporting
code. The machine.I2C class will still be available for software I2C.
Disabling all hardware I2C on an F4 board saves around 10,000 bytes of code
and 200 bytes of RAM.
