* [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 @ 2023-01-03 21:42 Heiko Stuebner 2023-01-04 10:06 ` Andrew Jones 2023-01-04 13:29 ` Conor Dooley 0 siblings, 2 replies; 9+ messages in thread From: Heiko Stuebner @ 2023-01-03 21:42 UTC (permalink / raw) To: linux-riscv, palmer Cc: christoph.muellner, conor, philipp.tomsich, ajones, heiko, Heiko Stuebner From: Heiko Stuebner <heiko.stuebner@vrull.eu> On the non-assembler-side wrapping alternative-macros inside other macros to prevent duplication of code works, as the end result will just be a string that gets fed to the asm instruction. In real assembler code, wrapping .macro blocks inside other .macro blocks brings more restrictions on usage and the optimization done by commit 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") results in a compile error like: ../arch/riscv/lib/strcmp.S: Assembler messages: ../arch/riscv/lib/strcmp.S:15: Error: too many positional arguments ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" ../arch/riscv/lib/strcmp.S:15: Error: attempt to move .org backwards Going back to the original code for the non-assembler-part makes that code work again. So this reverts the #ifdef ASSEMBLY part of that commit to the previous variant with duplicated base. Fixes: 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> --- I was of two minds about either to revert the full patch, or doing just this partial one for the ASSEMBLY part. I did go with this variant, as I still like the idea of deduplicating as much as possible :-) arch/riscv/include/asm/alternative-macros.h | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h index 7226e2462584..e7bdb2a510a4 100644 --- a/arch/riscv/include/asm/alternative-macros.h +++ b/arch/riscv/include/asm/alternative-macros.h @@ -44,9 +44,20 @@ ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c .endm +/* + * Using ALTERNATIVE_CFG inside ALTERNATIVE_CFG_2 results in compile errors. + * So the common code needs to stay duplicated. + */ .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ new_c_2, vendor_id_2, errata_id_2, enable_2 - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 +886 : + .option push + .option norvc + .option norelax + \old_c + .option pop +887 : + ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1 ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 .endm -- 2.35.1 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 2023-01-03 21:42 [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 Heiko Stuebner @ 2023-01-04 10:06 ` Andrew Jones 2023-01-04 14:11 ` Heiko Stübner 2023-01-04 13:29 ` Conor Dooley 1 sibling, 1 reply; 9+ messages in thread From: Andrew Jones @ 2023-01-04 10:06 UTC (permalink / raw) To: Heiko Stuebner Cc: linux-riscv, palmer, christoph.muellner, conor, philipp.tomsich, Heiko Stuebner On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > On the non-assembler-side wrapping alternative-macros inside other macros > to prevent duplication of code works, as the end result will just be a > string that gets fed to the asm instruction. > > In real assembler code, wrapping .macro blocks inside other .macro blocks > brings more restrictions on usage and the optimization done by > commit 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > results in a compile error like: > > ../arch/riscv/lib/strcmp.S: Assembler messages: > ../arch/riscv/lib/strcmp.S:15: Error: too many positional arguments > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > ../arch/riscv/lib/strcmp.S:15: Error: attempt to move .org backwards Ouch. I thought I had tested that, but looking now at my test code I see I only bothered to test ALTERNATIVE(), not ALTERNATIVE_2(). Adding the ALTERNATIVE_2() test, I can reproduce this. It appears the issue is that as macro arguments may be separated by commas or spaces, the old and new instruction macro arguments, which have spaces between their instructions and operands, get interpreted as extra macro arguments. There's probably no way to convince the macro otherwise, unfortunately. > > Going back to the original code for the non-assembler-part makes that > code work again. So this reverts the #ifdef ASSEMBLY part of that commit > to the previous variant with duplicated base. > > Fixes: 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > --- > I was of two minds about either to revert the full patch, or doing just > this partial one for the ASSEMBLY part. I did go with this variant, as I > still like the idea of deduplicating as much as possible :-) > > arch/riscv/include/asm/alternative-macros.h | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h > index 7226e2462584..e7bdb2a510a4 100644 > --- a/arch/riscv/include/asm/alternative-macros.h > +++ b/arch/riscv/include/asm/alternative-macros.h > @@ -44,9 +44,20 @@ > ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c > .endm > > +/* > + * Using ALTERNATIVE_CFG inside ALTERNATIVE_CFG_2 results in compile errors. > + * So the common code needs to stay duplicated. > + */ > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > new_c_2, vendor_id_2, errata_id_2, enable_2 > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 > +886 : > + .option push > + .option norvc > + .option norelax > + \old_c > + .option pop > +887 : We could still share this by creating another macro which only takes old_c, and then invoke that from both ALTERNATIVE_CFG and ALTERNATIVE_CFG_2, I think. > + ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1 > ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 > .endm > > -- > 2.35.1 > Thanks, drew _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 2023-01-04 10:06 ` Andrew Jones @ 2023-01-04 14:11 ` Heiko Stübner 2023-01-04 14:16 ` Heiko Stübner 0 siblings, 1 reply; 9+ messages in thread From: Heiko Stübner @ 2023-01-04 14:11 UTC (permalink / raw) To: Andrew Jones Cc: linux-riscv, palmer, christoph.muellner, conor, philipp.tomsich Hi Andrew, Am Mittwoch, 4. Januar 2023, 11:06:29 CET schrieb Andrew Jones: > On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > On the non-assembler-side wrapping alternative-macros inside other macros > > to prevent duplication of code works, as the end result will just be a > > string that gets fed to the asm instruction. > > > > In real assembler code, wrapping .macro blocks inside other .macro blocks > > brings more restrictions on usage and the optimization done by > > commit 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > > results in a compile error like: > > > > ../arch/riscv/lib/strcmp.S: Assembler messages: > > ../arch/riscv/lib/strcmp.S:15: Error: too many positional arguments > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > ../arch/riscv/lib/strcmp.S:15: Error: attempt to move .org backwards > > Ouch. I thought I had tested that, but looking now at my test code I see I > only bothered to test ALTERNATIVE(), not ALTERNATIVE_2(). Adding the > ALTERNATIVE_2() test, I can reproduce this. > > It appears the issue is that as macro arguments may be separated by commas > or spaces, the old and new instruction macro arguments, which have spaces > between their instructions and operands, get interpreted as extra macro > arguments. There's probably no way to convince the macro otherwise, > unfortunately. > > > > > Going back to the original code for the non-assembler-part makes that > > code work again. So this reverts the #ifdef ASSEMBLY part of that commit > > to the previous variant with duplicated base. > > > > Fixes: 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > > --- > > I was of two minds about either to revert the full patch, or doing just > > this partial one for the ASSEMBLY part. I did go with this variant, as I > > still like the idea of deduplicating as much as possible :-) > > > > arch/riscv/include/asm/alternative-macros.h | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h > > index 7226e2462584..e7bdb2a510a4 100644 > > --- a/arch/riscv/include/asm/alternative-macros.h > > +++ b/arch/riscv/include/asm/alternative-macros.h > > @@ -44,9 +44,20 @@ > > ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c > > .endm > > > > +/* > > + * Using ALTERNATIVE_CFG inside ALTERNATIVE_CFG_2 results in compile errors. > > + * So the common code needs to stay duplicated. > > + */ > > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > > new_c_2, vendor_id_2, errata_id_2, enable_2 > > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 > > +886 : > > + .option push > > + .option norvc > > + .option norelax > > + \old_c > > + .option pop > > +887 : > > We could still share this by creating another macro which only takes old_c, > and then invoke that from both ALTERNATIVE_CFG and ALTERNATIVE_CFG_2, I > think. hmm, so I was trying the change below, but then even ALTERNATIVE_CFG fails. I was looking for quite a while for some sort of typo I may have in that change, but so far haven't found any. Did you have a different solution in mind that may work? Heiko ------------- 8< ------------- diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h index e7bdb2a510a4..49c67fd94c57 100644 --- a/arch/riscv/include/asm/alternative-macros.h +++ b/arch/riscv/include/asm/alternative-macros.h @@ -33,13 +33,17 @@ .endif .endm -.macro ALTERNATIVE_CFG old_c, new_c, vendor_id, errata_id, enable -886 : +.macro ALT_OLD_CONTENT old_c .option push .option norvc .option norelax \old_c .option pop +.endm + +.macro ALTERNATIVE_CFG old_c, new_c, vendor_id, errata_id, enable +886 : + ALT_OLD_CONTENT \old_c 887 : ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c .endm @@ -51,11 +55,7 @@ .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ new_c_2, vendor_id_2, errata_id_2, enable_2 886 : - .option push - .option norvc - .option norelax - \old_c - .option pop + ALT_OLD_CONTENT \old_c 887 : ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1 ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 2023-01-04 14:11 ` Heiko Stübner @ 2023-01-04 14:16 ` Heiko Stübner 0 siblings, 0 replies; 9+ messages in thread From: Heiko Stübner @ 2023-01-04 14:16 UTC (permalink / raw) To: Andrew Jones Cc: linux-riscv, palmer, christoph.muellner, conor, philipp.tomsich Hi again, scratch this mail, I just saw the other one from 15:08 :-) Heiko Am Mittwoch, 4. Januar 2023, 15:11:38 CET schrieb Heiko Stübner: > Hi Andrew, > > Am Mittwoch, 4. Januar 2023, 11:06:29 CET schrieb Andrew Jones: > > On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote: > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > > > On the non-assembler-side wrapping alternative-macros inside other macros > > > to prevent duplication of code works, as the end result will just be a > > > string that gets fed to the asm instruction. > > > > > > In real assembler code, wrapping .macro blocks inside other .macro blocks > > > brings more restrictions on usage and the optimization done by > > > commit 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > > > results in a compile error like: > > > > > > ../arch/riscv/lib/strcmp.S: Assembler messages: > > > ../arch/riscv/lib/strcmp.S:15: Error: too many positional arguments > > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > > ../arch/riscv/lib/strcmp.S:15: Error: attempt to move .org backwards > > > > Ouch. I thought I had tested that, but looking now at my test code I see I > > only bothered to test ALTERNATIVE(), not ALTERNATIVE_2(). Adding the > > ALTERNATIVE_2() test, I can reproduce this. > > > > It appears the issue is that as macro arguments may be separated by commas > > or spaces, the old and new instruction macro arguments, which have spaces > > between their instructions and operands, get interpreted as extra macro > > arguments. There's probably no way to convince the macro otherwise, > > unfortunately. > > > > > > > > Going back to the original code for the non-assembler-part makes that > > > code work again. So this reverts the #ifdef ASSEMBLY part of that commit > > > to the previous variant with duplicated base. > > > > > > Fixes: 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > --- > > > I was of two minds about either to revert the full patch, or doing just > > > this partial one for the ASSEMBLY part. I did go with this variant, as I > > > still like the idea of deduplicating as much as possible :-) > > > > > > arch/riscv/include/asm/alternative-macros.h | 13 ++++++++++++- > > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > > > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h > > > index 7226e2462584..e7bdb2a510a4 100644 > > > --- a/arch/riscv/include/asm/alternative-macros.h > > > +++ b/arch/riscv/include/asm/alternative-macros.h > > > @@ -44,9 +44,20 @@ > > > ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c > > > .endm > > > > > > +/* > > > + * Using ALTERNATIVE_CFG inside ALTERNATIVE_CFG_2 results in compile errors. > > > + * So the common code needs to stay duplicated. > > > + */ > > > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > > > new_c_2, vendor_id_2, errata_id_2, enable_2 > > > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 > > > +886 : > > > + .option push > > > + .option norvc > > > + .option norelax > > > + \old_c > > > + .option pop > > > +887 : > > > > We could still share this by creating another macro which only takes old_c, > > and then invoke that from both ALTERNATIVE_CFG and ALTERNATIVE_CFG_2, I > > think. > > hmm, so I was trying the change below, but then even ALTERNATIVE_CFG fails. > I was looking for quite a while for some sort of typo I may have in that > change, but so far haven't found any. > > Did you have a different solution in mind that may work? > > > Heiko > > ------------- 8< ------------- > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h > index e7bdb2a510a4..49c67fd94c57 100644 > --- a/arch/riscv/include/asm/alternative-macros.h > +++ b/arch/riscv/include/asm/alternative-macros.h > @@ -33,13 +33,17 @@ > .endif > .endm > > -.macro ALTERNATIVE_CFG old_c, new_c, vendor_id, errata_id, enable > -886 : > +.macro ALT_OLD_CONTENT old_c > .option push > .option norvc > .option norelax > \old_c > .option pop > +.endm > + > +.macro ALTERNATIVE_CFG old_c, new_c, vendor_id, errata_id, enable > +886 : > + ALT_OLD_CONTENT \old_c > 887 : > ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c > .endm > @@ -51,11 +55,7 @@ > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > new_c_2, vendor_id_2, errata_id_2, enable_2 > 886 : > - .option push > - .option norvc > - .option norelax > - \old_c > - .option pop > + ALT_OLD_CONTENT \old_c > 887 : > ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1 > ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 > > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 2023-01-03 21:42 [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 Heiko Stuebner 2023-01-04 10:06 ` Andrew Jones @ 2023-01-04 13:29 ` Conor Dooley 2023-01-04 14:08 ` Andrew Jones 1 sibling, 1 reply; 9+ messages in thread From: Conor Dooley @ 2023-01-04 13:29 UTC (permalink / raw) To: Heiko Stuebner, palmer Cc: linux-riscv, palmer, christoph.muellner, philipp.tomsich, ajones, Heiko Stuebner [-- Attachment #1.1: Type: text/plain, Size: 3313 bytes --] On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote: > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 Hey Heiko/Palmer, Was a little hard to tell from Drew's mail if he was objecting to this variant of the patch, but FWIW s/decup/dedup/ if this gets applied as-is. Thanks, Conor. > On the non-assembler-side wrapping alternative-macros inside other macros > to prevent duplication of code works, as the end result will just be a > string that gets fed to the asm instruction. > > In real assembler code, wrapping .macro blocks inside other .macro blocks > brings more restrictions on usage and the optimization done by > commit 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > results in a compile error like: > > ../arch/riscv/lib/strcmp.S: Assembler messages: > ../arch/riscv/lib/strcmp.S:15: Error: too many positional arguments > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > ../arch/riscv/lib/strcmp.S:15: Error: attempt to move .org backwards > > Going back to the original code for the non-assembler-part makes that > code work again. So this reverts the #ifdef ASSEMBLY part of that commit > to the previous variant with duplicated base. > > Fixes: 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > --- > I was of two minds about either to revert the full patch, or doing just > this partial one for the ASSEMBLY part. I did go with this variant, as I > still like the idea of deduplicating as much as possible :-) > > arch/riscv/include/asm/alternative-macros.h | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h > index 7226e2462584..e7bdb2a510a4 100644 > --- a/arch/riscv/include/asm/alternative-macros.h > +++ b/arch/riscv/include/asm/alternative-macros.h > @@ -44,9 +44,20 @@ > ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c > .endm > > +/* > + * Using ALTERNATIVE_CFG inside ALTERNATIVE_CFG_2 results in compile errors. > + * So the common code needs to stay duplicated. > + */ > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > new_c_2, vendor_id_2, errata_id_2, enable_2 > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 > +886 : > + .option push > + .option norvc > + .option norelax > + \old_c > + .option pop > +887 : > + ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1 > ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 > .endm > > -- > 2.35.1 > > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 228 bytes --] [-- Attachment #2: Type: text/plain, Size: 161 bytes --] _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 2023-01-04 13:29 ` Conor Dooley @ 2023-01-04 14:08 ` Andrew Jones 2023-01-04 15:58 ` Heiko Stübner 0 siblings, 1 reply; 9+ messages in thread From: Andrew Jones @ 2023-01-04 14:08 UTC (permalink / raw) To: Conor Dooley Cc: Heiko Stuebner, palmer, linux-riscv, christoph.muellner, philipp.tomsich, Heiko Stuebner On Wed, Jan 04, 2023 at 01:29:25PM +0000, Conor Dooley wrote: > On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote: > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 > > Hey Heiko/Palmer, > > Was a little hard to tell from Drew's mail if he was objecting to this > variant of the patch, but FWIW s/decup/dedup/ if this gets applied > as-is. I wasn't completely objecting, but rather suggesting we still try to factor out what we can. However, after reading a bit more about macros I tried this .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ new_c_2, vendor_id_2, errata_id_2, enable_2 - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 + ALTERNATIVE_CFG "\old_c", "\new_c_1", \vendor_id_1, \errata_id_1, \enable_1 ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 .endm which appears to work for my simple test. Does that work for you too, Heiko? If so, then I think I'd prefer we do that. Also, we may want to add quotes to all macro arguments which may contain spaces, even if things seem to work now, e.g. the \new_c_2 argument passed to ALT_NEW_CONTENT. Thanks, drew > > Thanks, > Conor. > > > On the non-assembler-side wrapping alternative-macros inside other macros > > to prevent duplication of code works, as the end result will just be a > > string that gets fed to the asm instruction. > > > > In real assembler code, wrapping .macro blocks inside other .macro blocks > > brings more restrictions on usage and the optimization done by > > commit 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > > results in a compile error like: > > > > ../arch/riscv/lib/strcmp.S: Assembler messages: > > ../arch/riscv/lib/strcmp.S:15: Error: too many positional arguments > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:" > > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:" > > ../arch/riscv/lib/strcmp.S:15: Error: attempt to move .org backwards > > > > Going back to the original code for the non-assembler-part makes that > > code work again. So this reverts the #ifdef ASSEMBLY part of that commit > > to the previous variant with duplicated base. > > > > Fixes: 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2") > > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu> > > --- > > I was of two minds about either to revert the full patch, or doing just > > this partial one for the ASSEMBLY part. I did go with this variant, as I > > still like the idea of deduplicating as much as possible :-) > > > > arch/riscv/include/asm/alternative-macros.h | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h > > index 7226e2462584..e7bdb2a510a4 100644 > > --- a/arch/riscv/include/asm/alternative-macros.h > > +++ b/arch/riscv/include/asm/alternative-macros.h > > @@ -44,9 +44,20 @@ > > ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c > > .endm > > > > +/* > > + * Using ALTERNATIVE_CFG inside ALTERNATIVE_CFG_2 results in compile errors. > > + * So the common code needs to stay duplicated. > > + */ > > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > > new_c_2, vendor_id_2, errata_id_2, enable_2 > > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 > > +886 : > > + .option push > > + .option norvc > > + .option norelax > > + \old_c > > + .option pop > > +887 : > > + ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1 > > ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 > > .endm > > > > -- > > 2.35.1 > > > > > > _______________________________________________ > > linux-riscv mailing list > > linux-riscv@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-riscv _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 2023-01-04 14:08 ` Andrew Jones @ 2023-01-04 15:58 ` Heiko Stübner 2023-01-05 15:00 ` Andrew Jones 0 siblings, 1 reply; 9+ messages in thread From: Heiko Stübner @ 2023-01-04 15:58 UTC (permalink / raw) To: Conor Dooley, Andrew Jones Cc: palmer, linux-riscv, christoph.muellner, philipp.tomsich Hi again, Am Mittwoch, 4. Januar 2023, 15:08:33 CET schrieb Andrew Jones: > On Wed, Jan 04, 2023 at 01:29:25PM +0000, Conor Dooley wrote: > > On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote: > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > > RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 > > > > Hey Heiko/Palmer, > > > > Was a little hard to tell from Drew's mail if he was objecting to this > > variant of the patch, but FWIW s/decup/dedup/ if this gets applied > > as-is. > > I wasn't completely objecting, but rather suggesting we still try to > factor out what we can. However, after reading a bit more about macros > I tried this > > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > new_c_2, vendor_id_2, errata_id_2, enable_2 > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 > + ALTERNATIVE_CFG "\old_c", "\new_c_1", \vendor_id_1, \errata_id_1, \enable_1 > ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 > .endm > > which appears to work for my simple test. Does that work for you too, > Heiko? If so, then I think I'd prefer we do that. Also, we may want to add > quotes to all macro arguments which may contain spaces, even if things > seem to work now, e.g. the \new_c_2 argument passed to ALT_NEW_CONTENT. Yay ... adding quotes really seems to work - at least a GNU toolchain I guess that may have been the reason to move new_c at the end and making it vararg in ALT_NEW_CONTENT originally. And in fact with added quotes the varargs argument wouldn't be necessary anymore. Just removing the varargs argument alone results in Error: too many positional arguments erros, but wrapping the "\new_c*" in quotes lets it compile again. Heiko _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 2023-01-04 15:58 ` Heiko Stübner @ 2023-01-05 15:00 ` Andrew Jones 2023-01-05 16:14 ` Heiko Stübner 0 siblings, 1 reply; 9+ messages in thread From: Andrew Jones @ 2023-01-05 15:00 UTC (permalink / raw) To: Heiko Stübner Cc: Conor Dooley, palmer, linux-riscv, christoph.muellner, philipp.tomsich On Wed, Jan 04, 2023 at 04:58:00PM +0100, Heiko Stübner wrote: > Hi again, > > Am Mittwoch, 4. Januar 2023, 15:08:33 CET schrieb Andrew Jones: > > On Wed, Jan 04, 2023 at 01:29:25PM +0000, Conor Dooley wrote: > > > On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote: > > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > > > > RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 > > > > > > Hey Heiko/Palmer, > > > > > > Was a little hard to tell from Drew's mail if he was objecting to this > > > variant of the patch, but FWIW s/decup/dedup/ if this gets applied > > > as-is. > > > > I wasn't completely objecting, but rather suggesting we still try to > > factor out what we can. However, after reading a bit more about macros > > I tried this > > > > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > > new_c_2, vendor_id_2, errata_id_2, enable_2 > > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 > > + ALTERNATIVE_CFG "\old_c", "\new_c_1", \vendor_id_1, \errata_id_1, \enable_1 > > ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 > > .endm > > > > which appears to work for my simple test. Does that work for you too, > > Heiko? If so, then I think I'd prefer we do that. Also, we may want to add > > quotes to all macro arguments which may contain spaces, even if things > > seem to work now, e.g. the \new_c_2 argument passed to ALT_NEW_CONTENT. > > Yay ... adding quotes really seems to work - at least a GNU toolchain clang isn't complaining for my test either. > > I guess that may have been the reason to move new_c at the end and making > it vararg in ALT_NEW_CONTENT originally. > > And in fact with added quotes the varargs argument wouldn't be necessary > anymore. > > Just removing the varargs argument alone results in > Error: too many positional arguments > erros, but wrapping the "\new_c*" in quotes lets it compile again. Excellent news. Do you plan to post the patch? Or should I post with your Reported-by tag? Thanks, drew _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 2023-01-05 15:00 ` Andrew Jones @ 2023-01-05 16:14 ` Heiko Stübner 0 siblings, 0 replies; 9+ messages in thread From: Heiko Stübner @ 2023-01-05 16:14 UTC (permalink / raw) To: Andrew Jones Cc: Conor Dooley, palmer, linux-riscv, christoph.muellner, philipp.tomsich Am Donnerstag, 5. Januar 2023, 16:00:45 CET schrieb Andrew Jones: > On Wed, Jan 04, 2023 at 04:58:00PM +0100, Heiko Stübner wrote: > > Hi again, > > > > Am Mittwoch, 4. Januar 2023, 15:08:33 CET schrieb Andrew Jones: > > > On Wed, Jan 04, 2023 at 01:29:25PM +0000, Conor Dooley wrote: > > > > On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote: > > > > > From: Heiko Stuebner <heiko.stuebner@vrull.eu> > > > > > > > > > RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 > > > > > > > > Hey Heiko/Palmer, > > > > > > > > Was a little hard to tell from Drew's mail if he was objecting to this > > > > variant of the patch, but FWIW s/decup/dedup/ if this gets applied > > > > as-is. > > > > > > I wasn't completely objecting, but rather suggesting we still try to > > > factor out what we can. However, after reading a bit more about macros > > > I tried this > > > > > > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \ > > > new_c_2, vendor_id_2, errata_id_2, enable_2 > > > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1 > > > + ALTERNATIVE_CFG "\old_c", "\new_c_1", \vendor_id_1, \errata_id_1, \enable_1 > > > ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2 > > > .endm > > > > > > which appears to work for my simple test. Does that work for you too, > > > Heiko? If so, then I think I'd prefer we do that. Also, we may want to add > > > quotes to all macro arguments which may contain spaces, even if things > > > seem to work now, e.g. the \new_c_2 argument passed to ALT_NEW_CONTENT. > > > > Yay ... adding quotes really seems to work - at least a GNU toolchain > > clang isn't complaining for my test either. Nice > > > > > I guess that may have been the reason to move new_c at the end and making > > it vararg in ALT_NEW_CONTENT originally. > > > > And in fact with added quotes the varargs argument wouldn't be necessary > > anymore. > > > > Just removing the varargs argument alone results in > > Error: too many positional arguments > > erros, but wrapping the "\new_c*" in quotes lets it compile again. > > Excellent news. Do you plan to post the patch? Or should I post with your > Reported-by tag? I was planning on posting this today. Did the v2 an hour ago :-) Heiko _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-05 22:20 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-03 21:42 [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2 Heiko Stuebner 2023-01-04 10:06 ` Andrew Jones 2023-01-04 14:11 ` Heiko Stübner 2023-01-04 14:16 ` Heiko Stübner 2023-01-04 13:29 ` Conor Dooley 2023-01-04 14:08 ` Andrew Jones 2023-01-04 15:58 ` Heiko Stübner 2023-01-05 15:00 ` Andrew Jones 2023-01-05 16:14 ` Heiko Stübner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.