On Sun, Nov 24, 2024, 08:59 Jeff Law <jeffreya...@gmail.com> wrote:
>
>
> On 11/13/24 7:16 AM, Mariam Arutunian wrote:
>
> >
> >
> > To address this, I added code in |target-supports.exp| and modified the
> > relevant tests.
> > I've attached the patch. Could you please check whether it is correct?
> Just a few more notes.
>
> I revamped the changes to emit_crc. I think the right way to fix that
> riscv problem you were chasing was actually higher in the call chain.
>
> If you look at the setup code when you use the backend expanders, it
> looks like this:
>
> > /* Use target specific expansion if it exists.
> > Otherwise, generate table-based CRC. */
> > if (direct_internal_fn_supported_p (fn, tree_pair (data_type,
> result_type),
> > OPTIMIZE_FOR_SPEED))
> > {
> > class expand_operand ops[4];
> > create_call_lhs_operand (&ops[0], dest, TYPE_MODE (result_type));
> > create_input_operand (&ops[1], crc, TYPE_MODE (result_type));
> > create_input_operand (&ops[2], data, TYPE_MODE (data_type));
> > create_input_operand (&ops[3], polynomial, TYPE_MODE
> (result_type));
> > insn_code icode = convert_optab_handler (optab, TYPE_MODE
> (data_type),
> > TYPE_MODE (result_type));
> > expand_insn (icode, 4, ops);
> > assign_call_lhs (lhs, dest, &ops[0]);
> > }
>
>
> We need to do basically the same thing (at least for the return value)
> when we expand via a table. If you look in assign_call_lhs is has all
> the necessary bits to handle the promoted subreg case correctly.
>
> I've changed the table based expansion clause to look like this:
>
> > else
> > {
> > /* We're bypassing all the operand conversions that are done in the
> > case when we get an icode, operands and pass that off to
> expand_insn.
> >
> > That path has special case handling for promoted return values
> which
> > we must emulate here (is the same kind of special treatment ever
> > needed for input arguments here?).
> >
> > In particular we do not want to store directly into a promoted
> > SUBREG destination, instead store into a suitably sized
> pseudo. */
> > rtx orig_dest = dest;
> > if (SUBREG_P (dest) && SUBREG_PROMOTED_VAR_P (dest))
> > dest = gen_reg_rtx (GET_MODE (dest));
> >
> > /* If it's IFN_CRC generate bit-forward CRC. */
> > if (fn == IFN_CRC)
> > expand_crc_table_based (dest, crc, data, polynomial,
> > TYPE_MODE (data_type));
> > else
> > /* If it's IFN_CRC_REV generate bit-reversed CRC. */
> > expand_reversed_crc_table_based (dest, crc, data, polynomial,
> > TYPE_MODE (data_type),
> >
> generate_reflecting_code_standard);
> >
> > /* Now get the return value where it needs to be, taking care to
> > ensure it's promoted appropriately if the ABI demands it.
> >
> > Re-use assign_call_lhs to handle the details. */
> > class expand_operand ops[4];
> > create_call_lhs_operand (&ops[0], dest, TYPE_MODE (result_type));
> > ops[0].value = dest;
> > assign_call_lhs (lhs, orig_dest, &ops[0]);
> > }
>
>
> And I'm also starting to fix the word_size assumptions, particularly in
> the table expansion path. I know we used word_size to simplify stuff in
> the target bits and that's probably still the right thing to do in those
> target specific paths. But for the table lookup path we shouldn't
> really need to do that. By removing the word_size assumptions we also
> can avoid needing to make the CRC builtins conditional on target
> specific properties -- I'd really likely to avoid having the
> availability of the builtins be dependent on the target word size.
>
> In the testsuite, tests which use integers that don't fit in 16 bits
> need a magic marker so they they're not used on 16 bit integer targets.
>
> /* { dg-require-effective-target int32plus } */
>
>
> There were two tests using assert which we generally try to avoid.
> Instead we should just do a simple equality test and abort if the test
> is false. Also execution tests need to exit with zero status when they
> pass. I've fixed those problems as well.
>
> I've pushed the current state to users/jlaw/mariam-crc-branch. I
> haven't yet incorporated your risc-v testsuite fix yet though. I have
> done some rebasing/squashing of patches where it made sense. I'm hoping
> to get the various final nits cleaned up this week.
>
Thank you very much! I'll have a look.
Please let me know if there's anything specific you’d like me to address.
Thanks,
Mariam
Jeff
>
>
>
