On 7/12/24 14:52, Jeff Law wrote:
>> +(define_insn "*fclass<ANYF:mode><X:mode>"
>> + [(set (match_operand:X 0 "register_operand" "=r")
>> + (unspec [(match_operand:ANYF 1 "register_operand" " f")]
>> + UNSPEC_FCLASS))]
>> + "TARGET_HARD_FLOAT"
>> + "fclass.<fmt>\t%0,%1";
>> + [(set_attr "type" "fcmp")
>> + (set_attr "mode" "<UNITMODE>")])
>> +
>> +;; Implementation note:
>> +;; Indirection via the expander needed due to md syntax limitations.
>> +;; define_insn needs tighter mode check (X for operand0) requiring <X:mode>
>> +;; in name. This forces callers like isfinite to specify mode but can't
>> due
>> +;; to their own standard pattern name requirement. The additional expander
>> +;; with matching RTL template (but elided mode check) allows callers to use
>> +;; expander's name with actual asm coming from stricter define_insn.
>> +
>> +(define_expand "fclass<ANYF:mode>"
>> + [(set (match_operand 0 "register_operand" "=r")
>> + (unspec [(match_operand:ANYF 1 "register_operand" " f")]
>> + UNSPEC_FCLASS))]
>> + "TARGET_HARD_FLOAT")
> Note that for fclass, which isn't a standard pattern name, you have
> additional text in the same -- say for example a mode. It's just those
> standard names defined in md.texi where we have naming restrictions.
>
> The net is you can avoid the fclass expander entirely. So you can drop
> the '*' in the fclass pattern.
Note to myself (after verifying):
- dropping of '*' is also needed to be able to call them explicitly
from other expanders.
- '*' prefix patterns are purely for internal recog()
> Then call it as
> gen_fclass<ANYF:mode>di or gen_fclass<ANYF:mode>si in the is* expanders.
Meaning this requires callers to be
if (TARGET_64BIT)
emit_insn (gen_fclass<ANYF:mode>di (t, operands[1]));
else
emit_insn (gen_fclass<ANYF:mode>si (t, operands[1]));
since they can't do the following due to their own Std pattern name
reqmt lest redefinition issue.
emit_insn (gen_fclass<ANYF:mode><X:mode> (t, operands[1]));
>
>> + /* Allow SI for rv32/rv64 and DI for rv64
>> + Explicit mode check (vs. specfying modes in RTL template
>> match_operand)
>> + due to syntax limitations.
>> + - Specifying X would cause multiple definitions
>> + - And to disambuguate that requires adding <X:mode> to name which
>> + no longer matches the "standard pattern name". */
>> + if (GET_MODE (operands[0]) != SImode
>> + && GET_MODE (operands[0]) != word_mode)
>> + FAIL;
>> +
>> + rtx t = gen_reg_rtx (word_mode);
>> + rtx t_op0 = gen_reg_rtx (word_mode);
>> +
>> + emit_insn (gen_fclass<ANYF:mode> (t, operands[1]));
>> + riscv_emit_binary (AND, t, t, GEN_INT (0x7e));
>> + rtx cmp = gen_rtx_NE (word_mode, t, const0_rtx);
>> + emit_insn (gen_cstore<mode>4 (t_op0, cmp, t, const0_rtx));
>> +
>> + if (TARGET_64BIT)
>> + {
>> + t_op0 = gen_lowpart (SImode, t_op0);
>> + SUBREG_PROMOTED_VAR_P (t_op0) = 1;
>> + SUBREG_PROMOTED_SET (t_op0, SRP_SIGNED);
>> + }
>> +
>> + emit_move_insn (operands[0], t_op0);
>> + DONE;
>> +})
> So this is repeated nearly verbatim in all 3 is* expanders. Seems ripe
> for refactoring where you pass in the magic constant (which is all that
> seems to change across those code fragments).
Yeah I should have thought about it myself. The prior tri-modal return
was different but now clearly this can all be combined.
I was hoping to use int iterators, but it seems this will have be a
helper in riscv.cc which is called from 3 explicit expanders.
> Hate to ask for another spin, but the refactoring seems like its worth
> doing to avoid the duplication.
Not a problem, I'll eventually get the md syntax :-)
> And if we're going to spin again, might
> as well drop the fclass expander that we really don't need.
Of course.
-Vineet
