Re: [PATCH v1 3/3] arm64/sve: Skip flushing Z registers with 128 bit vectors

From: Mark Brown <broonie@kernel.org>
To: Dave P Martin <Dave.Martin@arm.com>
Cc: Catalin Marinas <Catalin.Marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v1 3/3] arm64/sve: Skip flushing Z registers with 128 bit vectors
Date: Mon, 10 May 2021 17:16:58 +0100	[thread overview]
Message-ID: <20210510161658.GC4496@sirena.org.uk> (raw)
In-Reply-To: <20210510150809.GC18631@e103592.cambridge.arm.com>

[-- Attachment #1.1: Type: text/plain, Size: 1992 bytes --]

On Mon, May 10, 2021 at 04:08:09PM +0100, Dave P Martin wrote:
> On Mon, May 10, 2021 at 01:23:48PM +0100, Mark Brown wrote:

> >  SYM_FUNC_START(sve_flush_live)
> > +	cbz		x0, 1f	// A VQ-1 of 0 is 128 bits so no extra Z state

> Should we worry about branch mispredicts here?  It may be in the noise,
> but I wonder whether it's worth considering use of alternatives here
> instead.

If people are happy adding an alternative we can definitely do that,
people seemed to want to avoid them in the past and at this point I
don't have concrete data to support how much of a win is but it seems
very likely that it'll have the best overall performance - systems that
only have 128 bit vectors will never have to worry about the non-shared
bits and...

> I have a suspicion that VL = 128 bits won't be common at runtime, except
> in the case of systems where the physical (or max usable) vector length
> (i.e., sve_max_vl) is 128 bits.  

...like you I expect that systems with more than 128 bits won't tend to
configure down to 128 bits.  At the minute it's kind of finger in the
air what the practical impact actually is though, quite a lot of
unresolved variables.

Given the recently announced requirement for SVE in v9 I'd expect that
we'll actually see quite a lot of 128 bit systems in the wild for at
least some period, like with our own Neoverse N2 cores.

> > +		unsigned long vq_minus_one =
> > +			sve_vq_from_vl(current->thread.sve_vl) - 1;
> > +		sve_set_vq(vq_minus_one);
> > +		sve_flush_live(vq_minus_one);

> Seems reasonable.  sve_flush_live() could alternatively be made a C
> function, with asm wrappers for sve_flush_{z,p,ffr} so that the
> conditional logic can be inlined -- but I can't see that it would
> improve the generated code much.  So I'd be happy with it to stay in
> this form.

Yeah, I faffed a bit with options but it seemed like the effort wasn't
going to be worth it, mainly inflating the size of the code change.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

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