On Wed, Oct 9, 2024 at 7:45 AM Jeff Law <jeffreya...@gmail.com> wrote:
> > > On 10/8/24 4:52 AM, Mariam Arutunian wrote: > > > > > > On Sun, Sep 29, 2024 at 9:08 PM Jeff Law <jeffreya...@gmail.com > > <mailto:jeffreya...@gmail.com>> wrote: > > > > > > > > On 9/13/24 5:05 AM, Mariam Arutunian wrote: > > > Add two new internal functions (IFN_CRC, IFN_CRC_REV), to provide > > faster > > > CRC generation. > > > One performs bit-forward and the other bit-reversed CRC > computation. > > > If CRC optabs are supported, they are used for the CRC > computation. > > > Otherwise, table-based CRC is generated. > > > The supported data and CRC sizes are 8, 16, 32, and 64 bits. > > > The polynomial is without the leading 1. > > > A table with 256 elements is used to store precomputed CRCs. > > > For the reflection of inputs and the output, a simple algorithm > > involving > > > SHIFT, AND, and OR operations is used. > > > > > > gcc/ > > > > > > * doc/md.texi (crc@var{m}@var{n}4, > > > crc_rev@var{m}@var{n}4): Document. > > > * expr.cc (calculate_crc): New function. > > > (assemble_crc_table): Likewise. > > > (generate_crc_table): Likewise. > > > (calculate_table_based_CRC): Likewise. > > > (emit_crc): Likewise. > > > (expand_crc_table_based): Likewise. > > > (gen_common_operation_to_reflect): Likewise. > > > (reflect_64_bit_value): Likewise. > > > (reflect_32_bit_value): Likewise. > > > (reflect_16_bit_value): Likewise. > > > (reflect_8_bit_value): Likewise. > > > (generate_reflecting_code_standard): Likewise. > > > (expand_reversed_crc_table_based): Likewise. > > > * expr.h (generate_reflecting_code_standard): New function > > declaration. > > > (expand_crc_table_based): Likewise. > > > (expand_reversed_crc_table_based): Likewise. > > > * internal-fn.cc: (crc_direct): Define. > > > (direct_crc_optab_supported_p): Likewise. > > > (expand_crc_optab_fn): New function > > > * internal-fn.def (CRC, CRC_REV): New internal functions. > > > * optabs.def (crc_optab, crc_rev_optab): New optabs. > > Looks pretty good. Just one question/comment: > > > > > > > > +void > > > +emit_crc (machine_mode crc_mode, rtx* crc, rtx* op0) > > > +{ > > > + if (GET_MODE_BITSIZE (crc_mode).to_constant () == 32 > > > + && GET_MODE_BITSIZE (word_mode) == 64) > > > + { > > > + rtx a_low = gen_lowpart (crc_mode, *crc); > > > + *crc = gen_rtx_SIGN_EXTEND (word_mode, a_low); > > > + } > > > + rtx tgt = *op0; > > > + if (word_mode != crc_mode) > > > + tgt = simplify_gen_subreg (word_mode, *op0, crc_mode, 0); > > > + emit_move_insn (tgt, *crc); > > > +} > > It seems to me that first clause ought to apply any time word mode is > > larger than crc mode rather than making it check 32/64 magic > constants. > > > > > > When I apply it whenever the word mode is larger than crc mode, on RISC- > > V the CRC-16 and CRC-8 tests fail. > We should work to understand that. Magic constants are generally to be > avoided. There's nothing inherent in this code where those constant > size values should be used. > > This is likely pointing to a problem either in how emit_crc is handling > those other cases or in the RISC-V expansion code. > > I think the problem mainly arises from how the *original* CRC function is compiled. If I use *uint32_t* crc; and write crc = (crc << 1) ^ 0x04C11DB7; the compiler applies *sign extension*. But if I use *uint16_t* crc; crc = (crc << 1) ^ 0x0DB7; it applies *zero **extension*. I suspect that for unsigned values zero extension is applied, but in this case (with uint32_t), sign extension is applied instead. Best regards, Mariam > Jeff >
