From 2f466c2939cf4a0909ae9c9a4aa34a7bcf4a6eae Mon Sep 17 00:00:00 2001 From: ISSOtm Date: Sat, 14 Mar 2020 14:43:09 +0100 Subject: [PATCH] Revamp macro arg system This should significantly improve performance: on pokecrystal builds, perf reported as much CPU time spent on `yyparse` as on `sym_UseNewMacroArgs` Measurements show ~6 seconds of improvement on that codebase. This also fixes #321, as a bonus, due to saner management! --- include/asm/fstack.h | 8 +- include/asm/macro.h | 24 +++--- include/helpers.h | 4 + src/asm/asmy.y | 21 +++-- src/asm/fstack.c | 68 +++++++--------- src/asm/globlex.c | 22 +++--- src/asm/lexer.c | 30 ++------ src/asm/macro.c | 167 ++++++++++++++++++---------------------- src/asm/symbol.c | 2 - test/asm/rept-shift.asm | 17 ++++ test/asm/rept-shift.err | 2 + test/asm/rept-shift.out | 1 + test/asm/unique-id.asm | 15 ++++ test/asm/unique-id.err | 19 +++++ test/asm/unique-id.out | 0 15 files changed, 213 insertions(+), 187 deletions(-) create mode 100644 test/asm/rept-shift.asm create mode 100644 test/asm/rept-shift.err create mode 100644 test/asm/rept-shift.out create mode 100644 test/asm/unique-id.asm create mode 100644 test/asm/unique-id.err create mode 100644 test/asm/unique-id.out diff --git a/include/asm/fstack.h b/include/asm/fstack.h index 961583aa..ace621d5 100644 --- a/include/asm/fstack.h +++ b/include/asm/fstack.h @@ -21,12 +21,15 @@ #include "types.h" +struct MacroArgs; + struct sContext { YY_BUFFER_STATE FlexHandle; struct sSymbol *pMacro; struct sContext *pNext; char tzFileName[_MAX_PATH + 1]; - char *tzMacroArgs[MAXMACROARGS + 1]; + struct MacroArgs *macroArgs; + uint32_t uniqueID; int32_t nLine; uint32_t nStatus; FILE *pFile; @@ -40,13 +43,12 @@ struct sContext { extern unsigned int nMaxRecursionDepth; void fstk_RunInclude(char *tzFileName); -void fstk_RunMacroArg(int32_t s); void fstk_Init(char *s); void fstk_Dump(void); void fstk_DumpToStr(char *buf, size_t len); void fstk_DumpStringExpansions(void); void fstk_AddIncludePath(char *s); -uint32_t fstk_RunMacro(char *s); +bool fstk_RunMacro(char *s, struct MacroArgs *args); void fstk_RunRept(uint32_t count, int32_t nReptLineNo); FILE *fstk_FindFile(char const *fname, char **incPathUsed); int32_t fstk_GetLine(void); diff --git a/include/asm/macro.h b/include/asm/macro.h index ffecd8c1..2f60ee68 100644 --- a/include/asm/macro.h +++ b/include/asm/macro.h @@ -10,19 +10,25 @@ #define RGBDS_MACRO_H #include +#include + +#include "asm/warning.h" #include "helpers.h" -void macro_AddNewArg(char const *s); -void macro_SaveCurrentArgs(char *save[]); -void macro_RestoreCurrentArgs(char *save[]); -void macro_UseNewArgs(void); -char *macro_FindArg(int32_t i); -void macro_UseCurrentArgs(void); -void macro_SetArgID(uint32_t nMacroCount); +struct MacroArgs; + +struct MacroArgs *macro_GetCurrentArgs(void); +struct MacroArgs *macro_NewArgs(void); +void macro_AppendArg(struct MacroArgs *args, char *s); +void macro_UseNewArgs(struct MacroArgs *args); +void macro_FreeArgs(struct MacroArgs *args); +char const *macro_GetArg(uint32_t i); + +uint32_t macro_GetUniqueID(void); +char const *macro_GetUniqueIDStr(void); +void macro_SetUniqueID(uint32_t id); void macro_ShiftCurrentArgs(void); uint32_t macro_NbArgs(void); -void macro_Init(void); - #endif diff --git a/include/helpers.h b/include/helpers.h index e6ad6d5a..f76a1db8 100644 --- a/include/helpers.h +++ b/include/helpers.h @@ -27,4 +27,8 @@ #define DEVELOP 0 #endif +/* Macros for stringification */ +#define STR(x) #x +#define EXPAND_AND_STR(x) STR(x) + #endif /* HELPERS_H */ diff --git a/src/asm/asmy.y b/src/asm/asmy.y index 262285ba..f97f89d6 100644 --- a/src/asm/asmy.y +++ b/src/asm/asmy.y @@ -496,6 +496,7 @@ static void strsubUTF8(char *dest, const char *src, uint32_t pos, uint32_t len) struct Expression sVal; int32_t nConstValue; struct SectionSpec sectSpec; + struct MacroArgs *macroArg; } %type relocexpr @@ -595,6 +596,8 @@ static void strsubUTF8(char *dest, const char *src, uint32_t pos, uint32_t len) %token T_SECT_WRAM0 T_SECT_VRAM T_SECT_ROMX T_SECT_ROM0 T_SECT_HRAM %token T_SECT_WRAMX T_SECT_SRAM T_SECT_OAM +%type macroargs; + %token T_Z80_ADC T_Z80_ADD T_Z80_AND %token T_Z80_BIT %token T_Z80_CALL T_Z80_CCF T_Z80_CP T_Z80_CPL @@ -699,17 +702,21 @@ macro : T_ID { yy_set_state(LEX_STATE_MACROARGS); } macroargs { yy_set_state(LEX_STATE_NORMAL); - if (!fstk_RunMacro($1)) + if (!fstk_RunMacro($1, $3)) fatalerror("Macro '%s' not defined", $1); } ; -macroargs : /* empty */ - | macroarg - | macroarg ',' macroargs -; - -macroarg : T_STRING { macro_AddNewArg($1); } +macroargs : /* empty */ { + $$ = macro_NewArgs(); + } + | T_STRING { + $$ = macro_NewArgs(); + macro_AppendArg($$, strdup($1)); + } + | macroargs ',' T_STRING { + macro_AppendArg($$, strdup($3)); + } ; pseudoop : equ diff --git a/src/asm/fstack.c b/src/asm/fstack.c index d7273bad..2f834df4 100644 --- a/src/asm/fstack.c +++ b/src/asm/fstack.c @@ -85,14 +85,14 @@ static void pushcontext(void) switch ((*ppFileStack)->nStatus = nCurrentStatus) { case STAT_isMacroArg: case STAT_isMacro: - macro_SaveCurrentArgs((*ppFileStack)->tzMacroArgs); + (*ppFileStack)->macroArgs = macro_GetCurrentArgs(); (*ppFileStack)->pMacro = pCurrentMacro; break; case STAT_isInclude: (*ppFileStack)->pFile = pCurrentFile; break; case STAT_isREPTBlock: - macro_SaveCurrentArgs((*ppFileStack)->tzMacroArgs); + (*ppFileStack)->macroArgs = macro_GetCurrentArgs(); (*ppFileStack)->pREPTBlock = pCurrentREPTBlock; (*ppFileStack)->nREPTBlockSize = nCurrentREPTBlockSize; (*ppFileStack)->nREPTBlockCount = nCurrentREPTBlockCount; @@ -102,6 +102,7 @@ static void pushcontext(void) default: fatalerror("%s: Internal error.", __func__); } + (*ppFileStack)->uniqueID = macro_GetUniqueID(); nLineNo = 0; } @@ -122,9 +123,7 @@ static int32_t popcontext(void) yy_scan_bytes(pCurrentREPTBlock, nCurrentREPTBlockSize); yy_switch_to_buffer(CurrentFlexHandle); - macro_UseCurrentArgs(); - macro_SetArgID(nMacroCount++); - macro_UseNewArgs(); + macro_SetUniqueID(nMacroCount++); /* Increment REPT count in file path */ pREPTIterationWritePtr = @@ -176,17 +175,29 @@ static int32_t popcontext(void) CurrentFlexHandle = pLastFile->FlexHandle; strcpy((char *)tzCurrentFileName, (char *)pLastFile->tzFileName); - switch (nCurrentStatus = pLastFile->nStatus) { + switch (pLastFile->nStatus) { + struct MacroArgs *args; + case STAT_isMacroArg: case STAT_isMacro: - macro_RestoreCurrentArgs(pLastFile->tzMacroArgs); + args = macro_GetCurrentArgs(); + if (nCurrentStatus == STAT_isMacro) { + macro_FreeArgs(args); + free(args); + } + macro_UseNewArgs(pLastFile->macroArgs); pCurrentMacro = pLastFile->pMacro; break; case STAT_isInclude: pCurrentFile = pLastFile->pFile; break; case STAT_isREPTBlock: - macro_RestoreCurrentArgs(pLastFile->tzMacroArgs); + args = macro_GetCurrentArgs(); + if (nCurrentStatus == STAT_isMacro) { + macro_FreeArgs(args); + free(args); + } + macro_UseNewArgs(pLastFile->macroArgs); pCurrentREPTBlock = pLastFile->pREPTBlock; nCurrentREPTBlockSize = pLastFile->nREPTBlockSize; nCurrentREPTBlockCount = pLastFile->nREPTBlockCount; @@ -195,6 +206,9 @@ static int32_t popcontext(void) default: fatalerror("%s: Internal error.", __func__); } + macro_SetUniqueID(pLastFile->uniqueID); + + nCurrentStatus = pLastFile->nStatus; nFileStackDepth--; @@ -422,19 +436,19 @@ void fstk_RunInclude(char *tzFileName) /* * Set up a macro for parsing */ -uint32_t fstk_RunMacro(char *s) +bool fstk_RunMacro(char *s, struct MacroArgs *args) { struct sSymbol *sym = sym_FindMacro(s); int nPrintedChars; if (sym == NULL || sym->pMacro == NULL) - return 0; + return false; pushcontext(); - macro_SetArgID(nMacroCount++); + macro_SetUniqueID(nMacroCount++); /* Minus 1 because there is a newline at the beginning of the buffer */ nLineNo = sym->nFileLine - 1; - macro_UseNewArgs(); + macro_UseNewArgs(args); nCurrentStatus = STAT_isMacro; nPrintedChars = snprintf(tzCurrentFileName, _MAX_PATH + 1, "%s::%s", sym->tzFileName, s); @@ -448,31 +462,7 @@ uint32_t fstk_RunMacro(char *s) strlen(pCurrentMacro->pMacro)); yy_switch_to_buffer(CurrentFlexHandle); - return 1; -} - -/* - * Set up a macroargument for parsing - */ -void fstk_RunMacroArg(int32_t s) -{ - char *sym; - - if (s == '@') - s = -1; - else - s -= '0'; - - sym = macro_FindArg(s); - - if (sym == NULL) - fatalerror("No such macroargument"); - - pushcontext(); - nCurrentStatus = STAT_isMacroArg; - snprintf(tzCurrentFileName, _MAX_PATH + 1, "%c", (uint8_t)s); - CurrentFlexHandle = yy_scan_bytes(sym, strlen(sym)); - yy_switch_to_buffer(CurrentFlexHandle); + return true; } /* @@ -505,10 +495,8 @@ void fstk_RunRept(uint32_t count, int32_t nReptLineNo) /* For error printing to make sense, fake nLineNo */ nCurrentREPTBodyLastLine = nLineNo; nLineNo = nReptLineNo; - macro_UseCurrentArgs(); pushcontext(); - macro_SetArgID(nMacroCount++); - macro_UseNewArgs(); + macro_SetUniqueID(nMacroCount++); nCurrentREPTBlockCount = count; nCurrentStatus = STAT_isREPTBlock; nCurrentREPTBlockSize = ulNewMacroSize; diff --git a/src/asm/globlex.c b/src/asm/globlex.c index 100b8df7..5044ed31 100644 --- a/src/asm/globlex.c +++ b/src/asm/globlex.c @@ -192,14 +192,14 @@ uint32_t ParseNumber(char *s, uint32_t size) * return a pointer to the rest of the macro arg. * Otherwise, return NULL. */ -char *AppendMacroArg(char whichArg, char *dest, size_t *destIndex) +char const *AppendMacroArg(char whichArg, char *dest, size_t *destIndex) { - char *marg; + char const *marg; if (whichArg == '@') - marg = macro_FindArg(-1); + marg = macro_GetUniqueIDStr(); else if (whichArg >= '1' && whichArg <= '9') - marg = macro_FindArg(whichArg - '0'); + marg = macro_GetArg(whichArg - '0'); else fatalerror("Invalid macro argument '\\%c' in symbol", whichArg); @@ -236,7 +236,7 @@ uint32_t ParseSymbol(char *src, uint32_t size) char dest[MAXSYMLEN + 1]; size_t srcIndex = 0; size_t destIndex = 0; - char *rest = NULL; + char const *rest = NULL; while (srcIndex < size) { char ch = src[srcIndex++]; @@ -302,11 +302,11 @@ uint32_t ParseSymbol(char *src, uint32_t size) uint32_t PutMacroArg(char *src, uint32_t size) { - char *s; + char const *s; yyskipbytes(size); if ((size == 2 && src[1] >= '1' && src[1] <= '9')) { - s = macro_FindArg(src[1] - '0'); + s = macro_GetArg(src[1] - '0'); if (s != NULL) yyunputstr(s); @@ -318,14 +318,14 @@ uint32_t PutMacroArg(char *src, uint32_t size) return 0; } -uint32_t PutUniqueArg(char *src, uint32_t size) +uint32_t PutUniqueID(char *src, uint32_t size) { (void)src; - char *s; + char const *s; yyskipbytes(size); - s = macro_FindArg(-1); + s = macro_GetUniqueIDStr(); if (s != NULL) yyunputstr(s); @@ -567,7 +567,7 @@ const struct sLexFloat tMacroArgToken = { }; const struct sLexFloat tMacroUniqueToken = { - PutUniqueArg, + PutUniqueID, T_LEX_MACROUNIQUE }; diff --git a/src/asm/lexer.c b/src/asm/lexer.c index cc65cdd0..ac2fbd64 100644 --- a/src/asm/lexer.c +++ b/src/asm/lexer.c @@ -583,33 +583,19 @@ struct sLexString *yylex_GetLongestFixed(void) size_t CopyMacroArg(char *dest, size_t maxLength, char c) { size_t i; - char *s; - int32_t argNum; + char const *s; - switch (c) { - case '1': - case '2': - case '3': - case '4': - case '5': - case '6': - case '7': - case '8': - case '9': - argNum = c - '0'; - break; - case '@': - argNum = -1; - break; - default: + if (c == '@') + s = macro_GetUniqueIDStr(); + else if (c >= '1' && c <= '9') + s = macro_GetArg(c - '0'); + else return 0; - } - - s = macro_FindArg(argNum); if (s == NULL) - fatalerror("Macro argument not defined"); + fatalerror("Macro argument '\\%c' not defined", c); + // TODO: `strncpy`, nay? for (i = 0; s[i] != 0; i++) { if (i >= maxLength) fatalerror("Macro argument too long to fit buffer"); diff --git a/src/asm/macro.c b/src/asm/macro.c index 71be9e6e..72de2091 100644 --- a/src/asm/macro.c +++ b/src/asm/macro.c @@ -8,111 +8,92 @@ #include "asm/macro.h" #include "asm/warning.h" -static char *currentmacroargs[MAXMACROARGS + 1]; -static char *newmacroargs[MAXMACROARGS + 1]; +struct MacroArgs { + char *args[MAXMACROARGS]; + unsigned int nbArgs; + unsigned int shift; +}; -void macro_AddNewArg(char const *s) +static struct MacroArgs defaultArgs = { .nbArgs = 0, .shift = 0 }; +static struct MacroArgs *macroArgs = &defaultArgs; +static uint32_t uniqueID = -1; +/* + * The initialization is somewhat harmful, since it is never used, but it + * guarantees the size of the buffer will be correct. I was unable to find a + * better solution, but if you have one, please feel free! + */ +static char uniqueIDBuf[] = "_" EXPAND_AND_STR(UINT32_MAX); +static char *uniqueIDPtr = NULL; + +struct MacroArgs *macro_GetCurrentArgs(void) { - int32_t i = 0; + return macroArgs; +} - while (i < MAXMACROARGS && newmacroargs[i] != NULL) - i++; +struct MacroArgs *macro_NewArgs(void) +{ + struct MacroArgs *args = malloc(sizeof(*args)); - if (i < MAXMACROARGS) { - if (s) - newmacroargs[i] = strdup(s); - else - newmacroargs[i] = NULL; + args->nbArgs = 0; + args->shift = 0; + return args; +} + +void macro_AppendArg(struct MacroArgs *args, char *s) +{ + if (args->nbArgs == MAXMACROARGS) + yyerror("A maximum of " EXPAND_AND_STR(MAXMACROARGS) + " arguments is allowed"); + args->args[args->nbArgs++] = s; +} + +void macro_UseNewArgs(struct MacroArgs *args) +{ + macroArgs = args; +} + +void macro_FreeArgs(struct MacroArgs *args) +{ + for (uint32_t i = 0; i < macroArgs->nbArgs; i++) + free(args->args[i]); +} + +char const *macro_GetArg(uint32_t i) +{ + uint32_t realIndex = i + macroArgs->shift - 1; + + return realIndex >= MAXMACROARGS ? NULL : macroArgs->args[realIndex]; +} + +uint32_t macro_GetUniqueID(void) +{ + return uniqueID; +} + +char const *macro_GetUniqueIDStr(void) +{ + return uniqueIDPtr; +} + +void macro_SetUniqueID(uint32_t id) +{ + uniqueID = id; + if (id == -1) { + uniqueIDPtr = NULL; } else { - yyerror("A maximum of %d arguments allowed", MAXMACROARGS); + /* The buffer is guaranteed to be the correct size */ + sprintf(uniqueIDBuf, "_%u", id); + uniqueIDPtr = uniqueIDBuf; } } -void macro_SaveCurrentArgs(char *save[]) -{ - int32_t i; - - for (i = 0; i <= MAXMACROARGS; i++) { - save[i] = currentmacroargs[i]; - currentmacroargs[i] = NULL; - } -} - -void macro_RestoreCurrentArgs(char *save[]) -{ - int32_t i; - - for (i = 0; i <= MAXMACROARGS; i++) { - free(currentmacroargs[i]); - currentmacroargs[i] = save[i]; - } -} - -void macro_UseNewArgs(void) -{ - int32_t i; - - for (i = 0; i <= MAXMACROARGS; i++) { - free(currentmacroargs[i]); - currentmacroargs[i] = newmacroargs[i]; - newmacroargs[i] = NULL; - } -} - -char *macro_FindArg(int32_t i) -{ - if (i == -1) - i = MAXMACROARGS + 1; - - assert(i >= 1); - - assert((size_t)(i - 1) - < sizeof(currentmacroargs) / sizeof(*currentmacroargs)); - - return currentmacroargs[i - 1]; -} - -void macro_UseCurrentArgs(void) -{ - int32_t i; - - for (i = 1; i <= MAXMACROARGS; i++) - macro_AddNewArg(macro_FindArg(i)); -} - -void macro_SetArgID(uint32_t nMacroCount) -{ - char s[256]; - - snprintf(s, sizeof(s) - 1, "_%u", nMacroCount); - newmacroargs[MAXMACROARGS] = strdup(s); -} - void macro_ShiftCurrentArgs(void) { - int32_t i; - - free(currentmacroargs[0]); - for (i = 0; i < MAXMACROARGS - 1; i++) - currentmacroargs[i] = currentmacroargs[i + 1]; - - currentmacroargs[MAXMACROARGS - 1] = NULL; + if (macroArgs->shift != macroArgs->nbArgs) + macroArgs->shift++; } uint32_t macro_NbArgs(void) { - uint32_t i = 0; - - while (currentmacroargs[i] && i < MAXMACROARGS) - i++; - - return i; -} - -void macro_Init(void) -{ - for (uint32_t i = 0; i < MAXMACROARGS; i++) { - currentmacroargs[i] = NULL; - newmacroargs[i] = NULL; - } + return macroArgs->nbArgs - macroArgs->shift; } diff --git a/src/asm/symbol.c b/src/asm/symbol.c index 166c7d78..914464d8 100644 --- a/src/asm/symbol.c +++ b/src/asm/symbol.c @@ -570,8 +570,6 @@ static inline char const *removeLeadingZeros(char const *ptr) */ void sym_Init(void) { - macro_Init(); - for (int32_t i = 0; i < HASHSIZE; i++) tHashedSymbols[i] = NULL; diff --git a/test/asm/rept-shift.asm b/test/asm/rept-shift.asm new file mode 100644 index 00000000..b5699db1 --- /dev/null +++ b/test/asm/rept-shift.asm @@ -0,0 +1,17 @@ +m: macro + PRINTT "\1 " + REPT 4 + SHIFT + ENDR + PRINTT "\1s!\n" + + ; Shifting a little more to check that over-shifting doesn't crash + SHIFT + SHIFT + REPT 256 + SHIFT + ENDR + PRINTT "\1\n" +endm + + m This, used, not, to, work diff --git a/test/asm/rept-shift.err b/test/asm/rept-shift.err new file mode 100644 index 00000000..531d3cd3 --- /dev/null +++ b/test/asm/rept-shift.err @@ -0,0 +1,2 @@ +ERROR: rept-shift.asm(17) -> rept-shift.asm::m(14): + Macro argument '\1' not defined diff --git a/test/asm/rept-shift.out b/test/asm/rept-shift.out new file mode 100644 index 00000000..c6dfe36c --- /dev/null +++ b/test/asm/rept-shift.out @@ -0,0 +1 @@ +This works! diff --git a/test/asm/unique-id.asm b/test/asm/unique-id.asm new file mode 100644 index 00000000..efa7618f --- /dev/null +++ b/test/asm/unique-id.asm @@ -0,0 +1,15 @@ +print EQUS "WARN \"\\@\"" + +m: macro + print + REPT 2 + print + ENDR + print +endm + ; TODO: Ideally we'd test now as well, but it'd cause a fatal error + ;print + m + ;print + m + print diff --git a/test/asm/unique-id.err b/test/asm/unique-id.err new file mode 100644 index 00000000..a39a3fa8 --- /dev/null +++ b/test/asm/unique-id.err @@ -0,0 +1,19 @@ +warning: unique-id.asm(12) -> unique-id.asm::m(4): [-Wuser] + _0 +warning: unique-id.asm(12) -> unique-id.asm::m(5) -> unique-id.asm::m::REPT~1(6): [-Wuser] + _1 +warning: unique-id.asm(12) -> unique-id.asm::m(5) -> unique-id.asm::m::REPT~2(6): [-Wuser] + _2 +warning: unique-id.asm(12) -> unique-id.asm::m(8): [-Wuser] + _0 +warning: unique-id.asm(14) -> unique-id.asm::m(4): [-Wuser] + _3 +warning: unique-id.asm(14) -> unique-id.asm::m(5) -> unique-id.asm::m::REPT~1(6): [-Wuser] + _4 +warning: unique-id.asm(14) -> unique-id.asm::m(5) -> unique-id.asm::m::REPT~2(6): [-Wuser] + _5 +warning: unique-id.asm(14) -> unique-id.asm::m(8): [-Wuser] + _3 +ERROR: unique-id.asm(15): + Macro argument '\@' not defined +while expanding symbol "print" diff --git a/test/asm/unique-id.out b/test/asm/unique-id.out new file mode 100644 index 00000000..e69de29b