| Message ID | 20190613161656.20765-4-julien.grall@arm.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | arm64/sve: First steps towards optimizing syscalls | expand |
On Thu, Jun 13, 2019 at 05:16:51PM +0100, Julien Grall wrote: > The current version of the macro "for" is only able to work when the > counter is used to generate registers using mnemonics. This is because [*] Is this backwards? Previously, we _can't_ substitute register numbers directly into proper instruction mnemonics: we have to use .insn with an expression to generate the opcode directly instead (possibly via a macro). With this change we can paste the _for number straight into human- readable assembler mnemonics. > gas is not able to evaluate the expression generated if used in > registers name (i.e x\n). Nit: maybe: "a register name" or "a register's name" > Gas offers a way to evaluate macro arguments by using % in front of > them under the alternate macro mode [1]. This reference is obviously useful to reviewers, but I'm not sure about the life expectancy of such a URL. It probably belongs after the tearoff line rather than in the commit message. > The implementation of "for" is updated to use the alternate macro mode > and %, so we can use the macro in more cases. As the alternate macro mode > may have side-effect, this is disabled when generating the body. Nit: side-effects Nit: I'd probably say "expanding the body", to be consistent with gas's own terminology. (These are pedantic, and inessential to fix.) > While it is enough to prefix the argument of the macro "__for_body" with > %, the arguments of "__for" are also prefixed to get a more bearable > value in case of compilation error. > > [1] https://sourceware.org/binutils/docs/as/Altmacro.html > > Suggested-by: Dave Martin <dave.martin@arm.com> > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > Changes in v2: > - Fix typo in the commit message > --- > arch/arm64/include/asm/fpsimdmacros.h | 10 +++++++--- > 1 file changed, 7 insertions(+), 3 deletions(-) > > diff --git a/arch/arm64/include/asm/fpsimdmacros.h b/arch/arm64/include/asm/fpsimdmacros.h > index 46843515d77b..e2ab77dd9b4f 100644 > --- a/arch/arm64/include/asm/fpsimdmacros.h > +++ b/arch/arm64/include/asm/fpsimdmacros.h > @@ -177,19 +177,23 @@ > > .macro __for from:req, to:req > .if (\from) == (\to) > - _for__body \from > + _for__body %\from > .else > - __for \from, (\from) + ((\to) - (\from)) / 2 > - __for (\from) + ((\to) - (\from)) / 2 + 1, \to > + __for %\from, %((\from) + ((\to) - (\from)) / 2) > + __for %((\from) + ((\to) - (\from)) / 2 + 1), %\to > .endif > .endm > > .macro _for var:req, from:req, to:req, insn:vararg > .macro _for__body \var:req > + .noaltmacro > \insn > + .altmacro > .endm > > + .altmacro > __for \from, \to > + .noaltmacro > > .purgem _for__body > .endm Looks OK to me. With [*] addressed as appropriate, and modulo others' comments (if any): Reviewed-by: Dave Martin <Dave.Martin@arm.com> Cheers ---Dave
Hi Dave, On 6/21/19 4:32 PM, Dave Martin wrote: > On Thu, Jun 13, 2019 at 05:16:51PM +0100, Julien Grall wrote: >> The current version of the macro "for" is only able to work when the >> counter is used to generate registers using mnemonics. This is because > > [*] Is this backwards? Previously, we _can't_ substitute register > numbers directly into proper instruction mnemonics: we have to use > .insn with an expression to generate the opcode directly instead > (possibly via a macro). Hmm, yes this is backwards. I want to s/only able/not able/. > > With this change we can paste the _for number straight into human- > readable assembler mnemonics. > >> gas is not able to evaluate the expression generated if used in >> registers name (i.e x\n). > > Nit: maybe: "a register name" or "a register's name" Ok. > >> Gas offers a way to evaluate macro arguments by using % in front of >> them under the alternate macro mode [1]. > > This reference is obviously useful to reviewers, but I'm not sure about > the life expectancy of such a URL. It probably belongs after the > tearoff line rather than in the commit message. Sure. > >> The implementation of "for" is updated to use the alternate macro mode >> and %, so we can use the macro in more cases. As the alternate macro mode >> may have side-effect, this is disabled when generating the body. > > Nit: side-effects > > Nit: I'd probably say "expanding the body", to be consistent with gas's > own terminology. > > (These are pedantic, and inessential to fix.) I have fixed them :). > >> While it is enough to prefix the argument of the macro "__for_body" with >> %, the arguments of "__for" are also prefixed to get a more bearable >> value in case of compilation error. >> >> [1] https://sourceware.org/binutils/docs/as/Altmacro.html >> >> Suggested-by: Dave Martin <dave.martin@arm.com> >> Signed-off-by: Julien Grall <julien.grall@arm.com> >> >> --- >> Changes in v2: >> - Fix typo in the commit message >> --- >> arch/arm64/include/asm/fpsimdmacros.h | 10 +++++++--- >> 1 file changed, 7 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm64/include/asm/fpsimdmacros.h b/arch/arm64/include/asm/fpsimdmacros.h >> index 46843515d77b..e2ab77dd9b4f 100644 >> --- a/arch/arm64/include/asm/fpsimdmacros.h >> +++ b/arch/arm64/include/asm/fpsimdmacros.h >> @@ -177,19 +177,23 @@ >> >> .macro __for from:req, to:req >> .if (\from) == (\to) >> - _for__body \from >> + _for__body %\from >> .else >> - __for \from, (\from) + ((\to) - (\from)) / 2 >> - __for (\from) + ((\to) - (\from)) / 2 + 1, \to >> + __for %\from, %((\from) + ((\to) - (\from)) / 2) >> + __for %((\from) + ((\to) - (\from)) / 2 + 1), %\to >> .endif >> .endm >> >> .macro _for var:req, from:req, to:req, insn:vararg >> .macro _for__body \var:req >> + .noaltmacro >> \insn >> + .altmacro >> .endm >> >> + .altmacro >> __for \from, \to >> + .noaltmacro >> >> .purgem _for__body >> .endm > > Looks OK to me. With [*] addressed as appropriate, and modulo others' > comments (if any): This is the new commit message (I have taken the opportunity to reflow it): commit 0dabd72ee773a62ec25fde289a60a9de908bf41b Author: Julien Grall <julien.grall@arm.com> Date: Wed Dec 5 14:44:19 2018 +0000 arm64/fpsimdmacros: Allow the macro "for" to be used in more cases The current version of the macro "for" is not able to work when the counter is used to generate registers using mnemonics. This is because gas is not able to evaluate the expression generated if used in register's name (i.e x\n). Gas offers a way to evaluate macro arguments by using % in front of them under the alternate macro mode. The implementation of "for" is updated to use the alternate macro mode and %, so we can use the macro in more cases. As the alternate macro mode may have side-effects, this is disabled when expanding the body. While it is enough to prefix the argument of the macro "__for_body" with %, the arguments of "__for" are also prefixed to get a more bearable value in case of compilation error. Suggested-by: Dave Martin <dave.martin@arm.com> Signed-off-by: Julien Grall <julien.grall@arm.com> --- For the alternate macro mode documentation, see: https://sourceware.org/binutils/docs/as/Altmacro.html > > Reviewed-by: Dave Martin <Dave.Martin@arm.com> Thank you! Cheers,
On Mon, Jun 24, 2019 at 05:10:02PM +0100, Julien Grall wrote: > Hi Dave, > > On 6/21/19 4:32 PM, Dave Martin wrote: > >On Thu, Jun 13, 2019 at 05:16:51PM +0100, Julien Grall wrote: > >>The current version of the macro "for" is only able to work when the > >>counter is used to generate registers using mnemonics. This is because > > > >[*] Is this backwards? Previously, we _can't_ substitute register > >numbers directly into proper instruction mnemonics: we have to use > >.insn with an expression to generate the opcode directly instead > >(possibly via a macro). > > Hmm, yes this is backwards. I want to s/only able/not able/. > > > > >With this change we can paste the _for number straight into human- > >readable assembler mnemonics. > > > >>gas is not able to evaluate the expression generated if used in > >>registers name (i.e x\n). > > > >Nit: maybe: "a register name" or "a register's name" > > Ok. > > > > >>Gas offers a way to evaluate macro arguments by using % in front of > >>them under the alternate macro mode [1]. > > > >This reference is obviously useful to reviewers, but I'm not sure about > >the life expectancy of such a URL. It probably belongs after the > >tearoff line rather than in the commit message. > > Sure. > > > > >>The implementation of "for" is updated to use the alternate macro mode > >>and %, so we can use the macro in more cases. As the alternate macro mode > >>may have side-effect, this is disabled when generating the body. > > > >Nit: side-effects > > > >Nit: I'd probably say "expanding the body", to be consistent with gas's > >own terminology. > > > >(These are pedantic, and inessential to fix.) > > I have fixed them :). > > > > >>While it is enough to prefix the argument of the macro "__for_body" with > >>%, the arguments of "__for" are also prefixed to get a more bearable > >>value in case of compilation error. > >> > >>[1] https://sourceware.org/binutils/docs/as/Altmacro.html > >> > >>Suggested-by: Dave Martin <dave.martin@arm.com> > >>Signed-off-by: Julien Grall <julien.grall@arm.com> > >> > >>--- > >> Changes in v2: > >> - Fix typo in the commit message > >>--- > >> arch/arm64/include/asm/fpsimdmacros.h | 10 +++++++--- > >> 1 file changed, 7 insertions(+), 3 deletions(-) > >> > >>diff --git a/arch/arm64/include/asm/fpsimdmacros.h b/arch/arm64/include/asm/fpsimdmacros.h > >>index 46843515d77b..e2ab77dd9b4f 100644 > >>--- a/arch/arm64/include/asm/fpsimdmacros.h > >>+++ b/arch/arm64/include/asm/fpsimdmacros.h > >>@@ -177,19 +177,23 @@ > >> .macro __for from:req, to:req > >> .if (\from) == (\to) > >>- _for__body \from > >>+ _for__body %\from > >> .else > >>- __for \from, (\from) + ((\to) - (\from)) / 2 > >>- __for (\from) + ((\to) - (\from)) / 2 + 1, \to > >>+ __for %\from, %((\from) + ((\to) - (\from)) / 2) > >>+ __for %((\from) + ((\to) - (\from)) / 2 + 1), %\to > >> .endif > >> .endm > >> .macro _for var:req, from:req, to:req, insn:vararg > >> .macro _for__body \var:req > >>+ .noaltmacro > >> \insn > >>+ .altmacro > >> .endm > >>+ .altmacro > >> __for \from, \to > >>+ .noaltmacro > >> .purgem _for__body > >> .endm > > > >Looks OK to me. With [*] addressed as appropriate, and modulo others' > >comments (if any): > > This is the new commit message (I have taken the opportunity to reflow it): > > commit 0dabd72ee773a62ec25fde289a60a9de908bf41b > Author: Julien Grall <julien.grall@arm.com> > Date: Wed Dec 5 14:44:19 2018 +0000 > > arm64/fpsimdmacros: Allow the macro "for" to be used in more cases > > The current version of the macro "for" is not able to work when the > counter is used to generate registers using mnemonics. This is because > gas is not able to evaluate the expression generated if used in > register's name (i.e x\n). > > Gas offers a way to evaluate macro arguments by using % in front of > them under the alternate macro mode. > > The implementation of "for" is updated to use the alternate macro mode > and %, so we can use the macro in more cases. As the alternate macro > mode may have side-effects, this is disabled when expanding the body. > > While it is enough to prefix the argument of the macro "__for_body" > with %, the arguments of "__for" are also prefixed to get a more > bearable value in case of compilation error. > > Suggested-by: Dave Martin <dave.martin@arm.com> > Signed-off-by: Julien Grall <julien.grall@arm.com> > > --- > > For the alternate macro mode documentation, see: > https://sourceware.org/binutils/docs/as/Altmacro.html > > > > >Reviewed-by: Dave Martin <Dave.Martin@arm.com> That looks fine to me. Cheers ---Dave
diff --git a/arch/arm64/include/asm/fpsimdmacros.h b/arch/arm64/include/asm/fpsimdmacros.h index 46843515d77b..e2ab77dd9b4f 100644 --- a/arch/arm64/include/asm/fpsimdmacros.h +++ b/arch/arm64/include/asm/fpsimdmacros.h @@ -177,19 +177,23 @@ .macro __for from:req, to:req .if (\from) == (\to) - _for__body \from + _for__body %\from .else - __for \from, (\from) + ((\to) - (\from)) / 2 - __for (\from) + ((\to) - (\from)) / 2 + 1, \to + __for %\from, %((\from) + ((\to) - (\from)) / 2) + __for %((\from) + ((\to) - (\from)) / 2 + 1), %\to .endif .endm .macro _for var:req, from:req, to:req, insn:vararg .macro _for__body \var:req + .noaltmacro \insn + .altmacro .endm + .altmacro __for \from, \to + .noaltmacro .purgem _for__body .endm
The current version of the macro "for" is only able to work when the counter is used to generate registers using mnemonics. This is because gas is not able to evaluate the expression generated if used in registers name (i.e x\n). Gas offers a way to evaluate macro arguments by using % in front of them under the alternate macro mode [1]. The implementation of "for" is updated to use the alternate macro mode and %, so we can use the macro in more cases. As the alternate macro mode may have side-effect, this is disabled when generating the body. While it is enough to prefix the argument of the macro "__for_body" with %, the arguments of "__for" are also prefixed to get a more bearable value in case of compilation error. [1] https://sourceware.org/binutils/docs/as/Altmacro.html Suggested-by: Dave Martin <dave.martin@arm.com> Signed-off-by: Julien Grall <julien.grall@arm.com> --- Changes in v2: - Fix typo in the commit message --- arch/arm64/include/asm/fpsimdmacros.h | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)