Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

On Thu, Apr 05, 2018 at 06:43:05PM +, Nick Desaulniers wrote:
> On Thu, Mar 22, 2018 at 4:58 PM Matthias Kaehlcke  wrote:
> 
> > El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:
> > > NAK.  There's a reason I didn't send my change upstream.
> > >
> > > As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> > > prefix tells gcc to pick the appropriate register width.  "x" makes it
> > > unconditionally use the entire 64-bit register width.  Just swapping out
> > > one for the other changes the macro's semantics.
> > >
> > > Unfortunately since this was breaking builds in android-4.14 and we
> > > didn't have an immediate-term fix, I bit the bullet and added the above
> > > commit -- but *only* as a short-term workaround.  For the one caller we
> > > currently have in 4.14.y, gcc was using the entire 64-bit width for all
> > > its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> > > might not be true if/when someone introduces other SMCCC 1.1 callers.
> > >
> > > Unfortunately I don't see a better way to deal with this than waiting
> > > for clang to support "r"-style constraints on ARM64.
> 
> > Thanks for the clarification! From the other thread
> > (https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
> > folks saw the option of a mergeable fix.
> 
> > Given the fact that clang support for kernel builds is still
> > recent/WIP I guess it's not the end of the world if we have to raise
> > the minimum clang version to 7.x for newer kernels.
> 
> 
> Manoj fixed this in:
> https://reviews.llvm.org/rL328829
> https://bugs.llvm.org/show_bug.cgi?id=36862
> 
> Looks set to ride the Clang 6.0 train.  mka@ if you're planning another
> state of the union email, it would be good to note the clang 6.0
> requirement for arm64.
> 
> Is there anything left to do here?

We should be good, unless somebody wants to look into a patch that
fixes clang pre-6.0.1 builds and doesn't look too ugly.

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

On Thu, Apr 05, 2018 at 06:43:05PM +, Nick Desaulniers wrote:
> On Thu, Mar 22, 2018 at 4:58 PM Matthias Kaehlcke  wrote:
> 
> > El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:
> > > NAK.  There's a reason I didn't send my change upstream.
> > >
> > > As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> > > prefix tells gcc to pick the appropriate register width.  "x" makes it
> > > unconditionally use the entire 64-bit register width.  Just swapping out
> > > one for the other changes the macro's semantics.
> > >
> > > Unfortunately since this was breaking builds in android-4.14 and we
> > > didn't have an immediate-term fix, I bit the bullet and added the above
> > > commit -- but *only* as a short-term workaround.  For the one caller we
> > > currently have in 4.14.y, gcc was using the entire 64-bit width for all
> > > its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> > > might not be true if/when someone introduces other SMCCC 1.1 callers.
> > >
> > > Unfortunately I don't see a better way to deal with this than waiting
> > > for clang to support "r"-style constraints on ARM64.
> 
> > Thanks for the clarification! From the other thread
> > (https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
> > folks saw the option of a mergeable fix.
> 
> > Given the fact that clang support for kernel builds is still
> > recent/WIP I guess it's not the end of the world if we have to raise
> > the minimum clang version to 7.x for newer kernels.
> 
> 
> Manoj fixed this in:
> https://reviews.llvm.org/rL328829
> https://bugs.llvm.org/show_bug.cgi?id=36862
> 
> Looks set to ride the Clang 6.0 train.  mka@ if you're planning another
> state of the union email, it would be good to note the clang 6.0
> requirement for arm64.
> 
> Is there anything left to do here?

We should be good, unless somebody wants to look into a patch that
fixes clang pre-6.0.1 builds and doesn't look too ugly.

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Nick Desaulniers]

On Thu, Mar 22, 2018 at 4:58 PM Matthias Kaehlcke  wrote:

> El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:
> > NAK.  There's a reason I didn't send my change upstream.
> >
> > As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> > prefix tells gcc to pick the appropriate register width.  "x" makes it
> > unconditionally use the entire 64-bit register width.  Just swapping out
> > one for the other changes the macro's semantics.
> >
> > Unfortunately since this was breaking builds in android-4.14 and we
> > didn't have an immediate-term fix, I bit the bullet and added the above
> > commit -- but *only* as a short-term workaround.  For the one caller we
> > currently have in 4.14.y, gcc was using the entire 64-bit width for all
> > its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> > might not be true if/when someone introduces other SMCCC 1.1 callers.
> >
> > Unfortunately I don't see a better way to deal with this than waiting
> > for clang to support "r"-style constraints on ARM64.

> Thanks for the clarification! From the other thread
> (https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
> folks saw the option of a mergeable fix.

> Given the fact that clang support for kernel builds is still
> recent/WIP I guess it's not the end of the world if we have to raise
> the minimum clang version to 7.x for newer kernels.


Manoj fixed this in:
https://reviews.llvm.org/rL328829
https://bugs.llvm.org/show_bug.cgi?id=36862

Looks set to ride the Clang 6.0 train.  mka@ if you're planning another
state of the union email, it would be good to note the clang 6.0
requirement for arm64.

Is there anything left to do here?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Nick Desaulniers]

On Thu, Mar 22, 2018 at 4:58 PM Matthias Kaehlcke  wrote:

> El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:
> > NAK.  There's a reason I didn't send my change upstream.
> >
> > As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> > prefix tells gcc to pick the appropriate register width.  "x" makes it
> > unconditionally use the entire 64-bit register width.  Just swapping out
> > one for the other changes the macro's semantics.
> >
> > Unfortunately since this was breaking builds in android-4.14 and we
> > didn't have an immediate-term fix, I bit the bullet and added the above
> > commit -- but *only* as a short-term workaround.  For the one caller we
> > currently have in 4.14.y, gcc was using the entire 64-bit width for all
> > its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> > might not be true if/when someone introduces other SMCCC 1.1 callers.
> >
> > Unfortunately I don't see a better way to deal with this than waiting
> > for clang to support "r"-style constraints on ARM64.

> Thanks for the clarification! From the other thread
> (https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
> folks saw the option of a mergeable fix.

> Given the fact that clang support for kernel builds is still
> recent/WIP I guess it's not the end of the world if we have to raise
> the minimum clang version to 7.x for newer kernels.


Manoj fixed this in:
https://reviews.llvm.org/rL328829
https://bugs.llvm.org/show_bug.cgi?id=36862

Looks set to ride the Clang 6.0 train.  mka@ if you're planning another
state of the union email, it would be good to note the clang 6.0
requirement for arm64.

Is there anything left to do here?
-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:

> On 03/22/2018 03:44 PM, Matthias Kaehlcke wrote:
> > El Thu, Mar 22, 2018 at 10:26:18PM + Nick Desaulniers ha dit:
> > 
> >> Note that a patch in this form has previously been implemented by:
> >>
> >> Andrey Konovalov :
> >> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
> >>
> >> and another by:
> >>
> >> Greg Hackmann :
> >> https://android-review.googlesource.com/c/kernel/common/+/645181
> >>
> >> If you used either as a reference, you may want to credit them with a
> >> `Suggested-by:` in the commit message.
> > 
> > Not really, but I think I prefer Greg's version over mine and might
> > use it in a respin if nobody raises objections.
> 
> NAK.  There's a reason I didn't send my change upstream.
> 
> As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> prefix tells gcc to pick the appropriate register width.  "x" makes it
> unconditionally use the entire 64-bit register width.  Just swapping out
> one for the other changes the macro's semantics.
> 
> Unfortunately since this was breaking builds in android-4.14 and we
> didn't have an immediate-term fix, I bit the bullet and added the above
> commit -- but *only* as a short-term workaround.  For the one caller we
> currently have in 4.14.y, gcc was using the entire 64-bit width for all
> its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> might not be true if/when someone introduces other SMCCC 1.1 callers.
> 
> Unfortunately I don't see a better way to deal with this than waiting
> for clang to support "r"-style constraints on ARM64.

Thanks for the clarification! From the other thread
(https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
folks saw the option of a mergeable fix.

Given the fact that clang support for kernel builds is still
recent/WIP I guess it's not the end of the world if we have to raise
the minimum clang version to 7.x for newer kernels.

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

El Thu, Mar 22, 2018 at 04:19:42PM -0700 Greg Hackmann ha dit:

> On 03/22/2018 03:44 PM, Matthias Kaehlcke wrote:
> > El Thu, Mar 22, 2018 at 10:26:18PM + Nick Desaulniers ha dit:
> > 
> >> Note that a patch in this form has previously been implemented by:
> >>
> >> Andrey Konovalov :
> >> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
> >>
> >> and another by:
> >>
> >> Greg Hackmann :
> >> https://android-review.googlesource.com/c/kernel/common/+/645181
> >>
> >> If you used either as a reference, you may want to credit them with a
> >> `Suggested-by:` in the commit message.
> > 
> > Not really, but I think I prefer Greg's version over mine and might
> > use it in a respin if nobody raises objections.
> 
> NAK.  There's a reason I didn't send my change upstream.
> 
> As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
> prefix tells gcc to pick the appropriate register width.  "x" makes it
> unconditionally use the entire 64-bit register width.  Just swapping out
> one for the other changes the macro's semantics.
> 
> Unfortunately since this was breaking builds in android-4.14 and we
> didn't have an immediate-term fix, I bit the bullet and added the above
> commit -- but *only* as a short-term workaround.  For the one caller we
> currently have in 4.14.y, gcc was using the entire 64-bit width for all
> its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
> might not be true if/when someone introduces other SMCCC 1.1 callers.
> 
> Unfortunately I don't see a better way to deal with this than waiting
> for clang to support "r"-style constraints on ARM64.

Thanks for the clarification! From the other thread
(https://lkml.org/lkml/2018/3/1/268) I had the impression that ARM
folks saw the option of a mergeable fix.

Given the fact that clang support for kernel builds is still
recent/WIP I guess it's not the end of the world if we have to raise
the minimum clang version to 7.x for newer kernels.

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Greg Hackmann]

On 03/22/2018 03:44 PM, Matthias Kaehlcke wrote:
> El Thu, Mar 22, 2018 at 10:26:18PM + Nick Desaulniers ha dit:
> 
>> Note that a patch in this form has previously been implemented by:
>>
>> Andrey Konovalov :
>> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
>>
>> and another by:
>>
>> Greg Hackmann :
>> https://android-review.googlesource.com/c/kernel/common/+/645181
>>
>> If you used either as a reference, you may want to credit them with a
>> `Suggested-by:` in the commit message.
> 
> Not really, but I think I prefer Greg's version over mine and might
> use it in a respin if nobody raises objections.

NAK.  There's a reason I didn't send my change upstream.

As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
prefix tells gcc to pick the appropriate register width.  "x" makes it
unconditionally use the entire 64-bit register width.  Just swapping out
one for the other changes the macro's semantics.

Unfortunately since this was breaking builds in android-4.14 and we
didn't have an immediate-term fix, I bit the bullet and added the above
commit -- but *only* as a short-term workaround.  For the one caller we
currently have in 4.14.y, gcc was using the entire 64-bit width for all
its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
might not be true if/when someone introduces other SMCCC 1.1 callers.

Unfortunately I don't see a better way to deal with this than waiting
for clang to support "r"-style constraints on ARM64.

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Greg Hackmann]

On 03/22/2018 03:44 PM, Matthias Kaehlcke wrote:
> El Thu, Mar 22, 2018 at 10:26:18PM + Nick Desaulniers ha dit:
> 
>> Note that a patch in this form has previously been implemented by:
>>
>> Andrey Konovalov :
>> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
>>
>> and another by:
>>
>> Greg Hackmann :
>> https://android-review.googlesource.com/c/kernel/common/+/645181
>>
>> If you used either as a reference, you may want to credit them with a
>> `Suggested-by:` in the commit message.
> 
> Not really, but I think I prefer Greg's version over mine and might
> use it in a respin if nobody raises objections.

NAK.  There's a reason I didn't send my change upstream.

As Marc pointed out (https://lkml.org/lkml/2018/3/16/987), the "r"
prefix tells gcc to pick the appropriate register width.  "x" makes it
unconditionally use the entire 64-bit register width.  Just swapping out
one for the other changes the macro's semantics.

Unfortunately since this was breaking builds in android-4.14 and we
didn't have an immediate-term fix, I bit the bullet and added the above
commit -- but *only* as a short-term workaround.  For the one caller we
currently have in 4.14.y, gcc was using the entire 64-bit width for all
its inputs anyway, so "r" vs. "x" didn't make a difference.  But that
might not be true if/when someone introduces other SMCCC 1.1 callers.

Unfortunately I don't see a better way to deal with this than waiting
for clang to support "r"-style constraints on ARM64.

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

El Thu, Mar 22, 2018 at 10:26:18PM + Nick Desaulniers ha dit:

> Note that a patch in this form has previously been implemented by:
> 
> Andrey Konovalov :
> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
> 
> and another by:
> 
> Greg Hackmann :
> https://android-review.googlesource.com/c/kernel/common/+/645181
> 
> If you used either as a reference, you may want to credit them with a
> `Suggested-by:` in the commit message.

Not really, but I think I prefer Greg's version over mine and might
use it in a respin if nobody raises objections.

> On Thu, Mar 22, 2018 at 2:28 PM Matthias Kaehlcke  wrote:
> > +#ifndef __clang__
> > +#define __reg__ "r"
> > +#else
> > +#define __reg__ "x"
> > +#endif
> 
> Can this be flipped to #ifdef __clang__ ?  having an if...else where the
> conditional negated is kind of funny.

Sure, my thought was "common case first", but I agree that the negated
condition isn't ideal either.

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Matthias Kaehlcke]

El Thu, Mar 22, 2018 at 10:26:18PM + Nick Desaulniers ha dit:

> Note that a patch in this form has previously been implemented by:
> 
> Andrey Konovalov :
> https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f
> 
> and another by:
> 
> Greg Hackmann :
> https://android-review.googlesource.com/c/kernel/common/+/645181
> 
> If you used either as a reference, you may want to credit them with a
> `Suggested-by:` in the commit message.

Not really, but I think I prefer Greg's version over mine and might
use it in a respin if nobody raises objections.

> On Thu, Mar 22, 2018 at 2:28 PM Matthias Kaehlcke  wrote:
> > +#ifndef __clang__
> > +#define __reg__ "r"
> > +#else
> > +#define __reg__ "x"
> > +#endif
> 
> Can this be flipped to #ifdef __clang__ ?  having an if...else where the
> conditional negated is kind of funny.

Sure, my thought was "common case first", but I agree that the negated
condition isn't ideal either.

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Nick Desaulniers]

Note that a patch in this form has previously been implemented by:

Andrey Konovalov :
https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f

and another by:

Greg Hackmann :
https://android-review.googlesource.com/c/kernel/common/+/645181

If you used either as a reference, you may want to credit them with a
`Suggested-by:` in the commit message.

On Thu, Mar 22, 2018 at 2:28 PM Matthias Kaehlcke  wrote:
> +#ifndef __clang__
> +#define __reg__ "r"
> +#else
> +#define __reg__ "x"
> +#endif

Can this be flipped to #ifdef __clang__ ?  having an if...else where the
conditional negated is kind of funny.

--
Thanks,
~Nick Desaulniers

Re: [PATCH] arm/arm64: smccc: Use xN for arm64 register constraints with clang

[Nick Desaulniers]

Note that a patch in this form has previously been implemented by:

Andrey Konovalov :
https://gist.github.com/xairy/ee11682ea86044a45c0291c528cd936f

and another by:

Greg Hackmann :
https://android-review.googlesource.com/c/kernel/common/+/645181

If you used either as a reference, you may want to credit them with a
`Suggested-by:` in the commit message.

On Thu, Mar 22, 2018 at 2:28 PM Matthias Kaehlcke  wrote:
> +#ifndef __clang__
> +#define __reg__ "r"
> +#else
> +#define __reg__ "x"
> +#endif

Can this be flipped to #ifdef __clang__ ?  having an if...else where the
conditional negated is kind of funny.

--
Thanks,
~Nick Desaulniers