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.
Jeff
