qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: "Matheus K. Ferst" <matheus.ferst@eldorado.org.br>
To: Richard Henderson <richard.henderson@linaro.org>,
	qemu-devel@nongnu.org, qemu-ppc@nongnu.org
Cc: lagarcia@br.ibm.com, bruno.larsen@eldorado.org.br,
	luis.pires@eldorado.org.br, f4bug@amsat.org,
	david@gibson.dropbear.id.au
Subject: Re: [PATCH v4 29/31] target/ppc: Implement cfuged instruction
Date: Thu, 13 May 2021 09:24:32 -0300	[thread overview]
Message-ID: <60f639cf-50e6-d967-a24b-a862b5a9e8fd@eldorado.org.br> (raw)
In-Reply-To: <49b2a677-a3cc-b944-dd4d-edd115c81039@linaro.org>

On 13/05/2021 08:31, Richard Henderson wrote:
> On 5/12/21 1:54 PM, matheus.ferst@eldorado.org.br wrote:
>> +    while (i) {
>> +        n = ctz64(mask);
>> +        if (n > i) {
>> +            n = i;
>> +        }
>> +
>> +        m = (1ll << n) - 1;
>> +        if (bit) {
>> +            right = ror64(right | (src & m), n);
>> +        } else {
>> +            left = ror64(left | (src & m), n);
>> +        }
>> +
>> +        src >>= n;
>> +        mask >>= n;
>> +        i -= n;
>> +        bit = !bit;
>> +        mask = ~mask;
>> +    }
>> +
>> +    if (bit) {
>> +        n = ctpop64(mask);
>> +    } else {
>> +        n = 64 - ctpop64(mask);
>> +    }
>> +
>> +    return left | (right >> n);
>> +}
> 
> This doesn't correspond to the algorithm presented in the manual.  Thus 
> this requires lots of extra commentary.
> 
> I guess I see how you're trying to process blocks at a time, instead of 
> single bits at a time.  But I don't think the merging of data into 
> "right" and "left" looks right.  I would have expected
> 
>      right = (right << n) | (src & m);
> 
> and similarly for left.
> 
> It doesn't look like that the ctpop at the end is correct, given how 
> mask has been modified.  I would have thought that
> 
>      n = ctpop64(orig_mask);
>      return (left << n) | right;
> 
> would be the correct answer.
> 
> I could be wrong about the above, but that's what the missing commentary 
> should have helped me understand.
> 

It sure worth more comments. Yes, the idea is to process in blocks, and 
we negate the mask to avoid deciding between ctz and cto inside the 
loop. We use rotate instead of shift so it don't change the number of 
zeros and ones, and then we don't need orig_mask.

You'll find my test cases for cfuged and vcfuged on 
https://github.com/PPC64/qemu/blob/ferst-tcg-cfuged/tests/tcg/ppc64le/ . 
I got the same results by running them with this implementation and with 
the Power10 Functional Simulator.

>> +static bool trans_CFUGED(DisasContext *ctx, arg_X *a)
>> +{
>> +    REQUIRE_64BIT(ctx);
>> +    REQUIRE_INSNS_FLAGS2(ctx, ISA310);
>> +#if defined(TARGET_PPC64)
>> +    gen_helper_cfuged(cpu_gpr[a->ra], cpu_gpr[a->rt], cpu_gpr[a->rb]);
>> +#else
>> +    gen_invalid(ctx);
>> +#endif
>> +    return true;
>> +}
> 
> Given that this helper will also be used by vcfuged, there's no point in 
> hiding it in a TARGET_PPC64 block, and thus you can drop the ifdefs.
> 
> 
> r~
> 

If I remove it, the build for ppc will fail, because cpu_gpr is declared 
as TCGv, and the helper uses i64 to match {get,set}_cpu_vsr{l,h}. 
REQUIRE_64BIT makes the helper call unreachable for ppc, but it's a 
runtime check. At build time, the compiler will check the types anyway, 
and give us an error.

-- 
Matheus K. Ferst
Instituto de Pesquisas ELDORADO <http://www.eldorado.org.br/>
Analista de Software Júnior
Aviso Legal - Disclaimer <https://www.eldorado.org.br/disclaimer.html>


  reply	other threads:[~2021-05-13 12:38 UTC|newest]

Thread overview: 56+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-12 18:54 [PATCH v4 00/31] Base for adding PowerPC 64-bit instructions matheus.ferst
2021-05-12 18:54 ` [PATCH v4 01/31] target/ppc: Add cia field to DisasContext matheus.ferst
2021-05-13  4:03   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 02/31] target/ppc: Split out decode_legacy matheus.ferst
2021-05-13  4:03   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 03/31] target/ppc: Move DISAS_NORETURN setting into gen_exception* matheus.ferst
2021-05-13  4:06   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 04/31] target/ppc: Remove special case for POWERPC_SYSCALL matheus.ferst
2021-05-13  4:06   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 05/31] target/ppc: Remove special case for POWERPC_EXCP_TRAP matheus.ferst
2021-05-13  4:07   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 06/31] target/ppc: Simplify gen_debug_exception matheus.ferst
2021-05-13  4:08   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 07/31] target/ppc: Introduce DISAS_{EXIT,CHAIN}{,_UPDATE} matheus.ferst
2021-05-13  4:10   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 08/31] target/ppc: Replace POWERPC_EXCP_SYNC with DISAS_EXIT matheus.ferst
2021-05-12 19:31   ` Bruno Piazera Larsen
2021-05-13  4:11   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 09/31] target/ppc: Remove unnecessary gen_io_end calls matheus.ferst
2021-05-13  4:12   ` David Gibson
2021-05-12 18:54 ` [PATCH v4 10/31] target/ppc: Introduce gen_icount_io_start matheus.ferst
2021-05-12 19:21   ` Matheus K. Ferst
2021-05-13  4:14     ` David Gibson
2021-05-12 18:54 ` [PATCH v4 11/31] target/ppc: Replace POWERPC_EXCP_STOP with DISAS_EXIT_UPDATE matheus.ferst
2021-05-12 18:54 ` [PATCH v4 12/31] target/ppc: Replace POWERPC_EXCP_BRANCH with DISAS_NORETURN matheus.ferst
2021-05-12 18:54 ` [PATCH v4 13/31] target/ppc: Remove DisasContext.exception matheus.ferst
2021-05-12 18:54 ` [PATCH v4 14/31] target/ppc: Move single-step check to ppc_tr_tb_stop matheus.ferst
2021-05-12 18:54 ` [PATCH v4 15/31] target/ppc: Tidy exception vs exit_tb matheus.ferst
2021-05-12 18:54 ` [PATCH v4 16/31] target/ppc: Mark helper_raise_exception* as noreturn matheus.ferst
2021-05-12 18:54 ` [PATCH v4 17/31] target/ppc: Use translator_loop_temp_check matheus.ferst
2021-05-12 19:45   ` Bruno Piazera Larsen
2021-05-12 18:54 ` [PATCH v4 18/31] target/ppc: Introduce macros to check isa extensions matheus.ferst
2021-05-12 18:54 ` [PATCH v4 19/31] target/ppc: Move page crossing check to ppc_tr_translate_insn matheus.ferst
2021-05-12 18:54 ` [PATCH v4 20/31] target/ppc: Add infrastructure for prefixed insns matheus.ferst
2021-05-12 18:54 ` [PATCH v4 21/31] target/ppc: Move ADDI, ADDIS to decodetree, implement PADDI matheus.ferst
2021-05-12 18:54 ` [PATCH v4 22/31] target/ppc: Implement PNOP matheus.ferst
2021-05-13 10:37   ` Richard Henderson
2021-05-12 18:54 ` [PATCH v4 23/31] TCG: add tcg_constant_tl matheus.ferst
2021-05-13 10:42   ` Richard Henderson
2021-05-12 18:54 ` [PATCH v4 24/31] target/ppc: Move D/DS/X-form integer loads to decodetree matheus.ferst
2021-05-12 18:54 ` [PATCH v4 25/31] target/ppc: Implement prefixed integer load instructions matheus.ferst
2021-05-13 10:50   ` Richard Henderson
2021-05-12 18:54 ` [PATCH v4 26/31] target/ppc: Move D/DS/X-form integer stores to decodetree matheus.ferst
2021-05-12 18:54 ` [PATCH v4 27/31] target/ppc: Implement prefixed integer store instructions matheus.ferst
2021-05-12 18:54 ` [PATCH v4 28/31] target/ppc: Implement setbc/setbcr/stnbc/setnbcr instructions matheus.ferst
2021-05-13 11:01   ` Richard Henderson
2021-05-13 11:43     ` Matheus K. Ferst
2021-05-12 18:54 ` [PATCH v4 29/31] target/ppc: Implement cfuged instruction matheus.ferst
2021-05-13 11:31   ` Richard Henderson
2021-05-13 12:24     ` Matheus K. Ferst [this message]
2021-05-14  0:01       ` Richard Henderson
2021-05-12 18:54 ` [PATCH v4 30/31] target/ppc: Implement vcfuged instruction matheus.ferst
2021-05-13 11:36   ` Richard Henderson
2021-05-12 18:54 ` [PATCH v4 31/31] target/ppc: Move addpcis to decodetree matheus.ferst
2021-05-13 11:40   ` Richard Henderson
2021-05-13  4:22 ` [PATCH v4 00/31] Base for adding PowerPC 64-bit instructions David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=60f639cf-50e6-d967-a24b-a862b5a9e8fd@eldorado.org.br \
    --to=matheus.ferst@eldorado.org.br \
    --cc=bruno.larsen@eldorado.org.br \
    --cc=david@gibson.dropbear.id.au \
    --cc=f4bug@amsat.org \
    --cc=lagarcia@br.ibm.com \
    --cc=luis.pires@eldorado.org.br \
    --cc=qemu-devel@nongnu.org \
    --cc=qemu-ppc@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --subject='Re: [PATCH v4 29/31] target/ppc: Implement cfuged instruction' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
on how to clone and mirror all data and code used for this inbox