Re: [PATCH v10 03/16] target/riscv: clwz must ignore high bits (use shift-left & changed logic)

From: Philipp Tomsich <philipp.tomsich@vrull.eu>
To: Richard Henderson <richard.henderson@linaro.org>
Cc: Kito Cheng <kito.cheng@sifive.com>,
	Alistair Francis <alistair.francis@wdc.com>,
	qemu-devel@nongnu.org
Subject: Re: [PATCH v10 03/16] target/riscv: clwz must ignore high bits (use shift-left & changed logic)
Date: Sun, 5 Sep 2021 12:01:31 +0300	[thread overview]
Message-ID: <CAAeLtUAgr9r2aBV+M8jVE7J0DG2U4-EjxAEfY1adhQ_XCfT5ZQ@mail.gmail.com> (raw)
In-Reply-To: <3e608998-3270-cf41-66b5-32158db99de0@linaro.org>

On Sun 5. Sep 2021 at 11:11, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 9/4/21 10:35 PM, Philipp Tomsich wrote:
> > Assume clzw being executed on a register that is not sign-extended, such
> > as for the following sequence that uses (1ULL << 63) | 392 as the operand
> > to clzw:
> >       bseti   a2, zero, 63
> >       addi    a2, a2, 392
> >       clzw    a3, a2
> > The correct result of clzw would be 23, but the current implementation
> > returns -32 (as it performs a 64bit clz, which results in 0 leading zero
> > bits, and then subtracts 32).
> >
> > Fix this by changing the implementation to:
> >   1. shift the original register up by 32
> >   2. performs a target-length (64bit) clz
> >   3. return 32 if no bits are set
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> > Changes in v10:
> > - New patch, fixing correctnes for clzw called on a register with undefined
> >    (as in: not properly sign-extended) upper bits.
>
> But we have
>
>      return gen_unary(ctx, a, EXT_ZERO, gen_clzw);
>
> should *not* be undefined.  Ah, what's missing is
>
>      ctx->w = true;
>
> within trans_clzw to cause the extend to take effect.
>
> There are a few other "w" functions that are missing that set, though they use EXT_NONE so
> there is no visible bug, it would probably be best to set w anyway.

I had originally considered that (it would have to be ctx->w = true;
and EXT_SIGN),
but that has the side-effect of performing an extension on the result
of the clzw as
well (i.e. bot get_gpr and set_gpr are extended).

However, this is not what clzw does: it only ignores the upper bits
and then produces
a result in the value-range [0..32]. An extension on the result of
either a clz or clzw
is never needed (we have been fighting that problem in GCC and had to
use patterns
for the combiner), so I don't want to introduce this inefficiency in qemu.

If you have EXT_SIGN (or _ZERO), we will end up with 2 additional
extensions (one
on the argument, one on the result) in addition to the 2 other tcg
operations that we
need (i.e. clzi/subi for the extending case, shli/clzi for the proposed fix).

So I believe that this commit is the best fix:
1. It doesn't mark this as a w-form (i.e. ignores the high-bits on the
input _and_
extends the output), as it only treats the input as 32bit, but the
output is xlen.
2. It doesn't introduce any unnecessary extensions, but handles the
case in-place.

Philipp.