Re: [RFC PATCH v2 3/8] arm64/fpsimdmacros: Allow the macro "for" to be used in more cases

From: Dave Martin <Dave.Martin@arm.com>
To: Julien Grall <julien.grall@arm.com>
Cc: Anton.Kirilov@arm.com, catalin.marinas@arm.com,
	will.deacon@arm.com, oleg@redhat.com, zhang.lei@jp.fujitsu.com,
	alex.bennee@linaro.org, linux-arm-kernel@lists.infradead.org,
	Daniel.Kiss@arm.com
Subject: Re: [RFC PATCH v2 3/8] arm64/fpsimdmacros: Allow the macro "for" to be used in more cases
Date: Tue, 25 Jun 2019 10:35:42 +0100	[thread overview]
Message-ID: <20190625093540.GW2790@e103592.cambridge.arm.com> (raw)
In-Reply-To: <8ba85821-3734-9e3c-5696-bc5e315a28c4@arm.com>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel