On Tue, May 21, 2024 at 5:46 AM Alexander Monakov <amona...@ispras.ru> wrote:
>
>
> Hello!
>
> I looked at ternlog a bit last year, so I'd like to offer some drive-by
> comments. If you want to tackle them in a follow-up patch, or leave for
> someone else to handle, please let me know.
>
> On Fri, 17 May 2024, Roger Sayle wrote:
>
> > This revised patch has been tested on x86_64-pc-linux-gnu with make
> > bootstrap
> > and make -k check, both with and without --target_board=unix{-m32}
> > with no new failures. Ok for mainline?
>
> Just to make sure: no new tests for the new tricks?
>
> > --- a/gcc/config/i386/i386-expand.cc
> > +++ b/gcc/config/i386/i386-expand.cc
> > +/* Determine the ternlog immediate index that implements 3-operand
> > + ternary logic expression OP. This uses and modifies the 3 element
> > + array ARGS to record and check the leaves, either 3 REGs, or 2 REGs
> > + and MEM. Returns an index between 0 and 255 for a valid ternlog,
> > + or -1 if the expression isn't suitable. */
> > +
> > +int
> > +ix86_ternlog_idx (rtx op, rtx *args)
> > +{
> > + int idx0, idx1;
> > +
> > + if (!op)
> > + return -1;
> > +
> > + switch (GET_CODE (op))
> > + {
> > + case REG:
> > + if (!args[0])
> > + {
> > + args[0] = op;
> > + return 0xf0;
>
> From readability perspective, I wonder if it's nicer to have something like
>
> enum {
> TERNLOG_A = 0xf0,
> TERNLOG_B = 0xcc,
> TERNLOG_C = 0xaa
> }
>
> and then use them to build the immediates.
>
> > + }
> > + if (REGNO (op) == REGNO (args[0]))
> > + return 0xf0;
> > + if (!args[1])
> > + {
> > + args[1] = op;
> > + return 0xcc;
> > + }
> [snip]
> > +
> > +/* Return TRUE if OP (in mode MODE) is the leaf of a ternary logic
> > + expression, such as a register or a memory reference. */
> > +
> > +bool
> > +ix86_ternlog_leaf_p (rtx op, machine_mode mode)
> > +{
> > + /* We can't use memory_operand here, as it may return a different
> > + value before and after reload (for volatile MEMs) which creates
> > + problems splitting instructions. */
> > + return register_operand (op, mode)
> > + || MEM_P (op)
> > + || GET_CODE (op) == CONST_VECTOR
> > + || bcst_mem_operand (op, mode);
>
> Did your editor automatically indent this correctly for you? I think
> usually such expressions have outer parenthesis.
>
> > +}
> [snip]
> > +/* Expand a 3-operand ternary logic expression. Return TARGET. */
> > +rtx
> > +ix86_expand_ternlog (machine_mode mode, rtx op0, rtx op1, rtx op2, int idx,
> > + rtx target)
> > +{
> > + rtx tmp0, tmp1, tmp2;
> > +
> > + if (!target)
> > + target = gen_reg_rtx (mode);
> > +
> > + /* Canonicalize ternlog index for degenerate (duplicated) operands. */
>
> But this only canonicalizes the case of triplicated operands, and does nothing
> if two operands are duplicates of each other, and the third is distinct.
> Handling that would complicate the already large patch a lot though.
I think it's handled below,
tmp0 = register_operand (op0, mode) ? op0 : force_reg (mode, op0);
if (GET_MODE (tmp0) != mode)
tmp0 = gen_lowpart (mode, tmp0);
if (!op1 || rtx_equal_p (op0, op1)) ---- here
tmp1 = copy_rtx (tmp0);
else if (!register_operand (op1, mode))
tmp1 = force_reg (mode, op1);
else
tmp1 = op1;
if (GET_MODE (tmp1) != mode)
tmp1 = gen_lowpart (mode, tmp1);
if (!op2 || rtx_equal_p (op0, op2)) ---------- and here.
tmp2 = copy_rtx (tmp0);
else if (rtx_equal_p (op1, op2))
tmp2 = copy_rtx (tmp1);
else if (GET_CODE (op2) == CONST_VECTOR)
{
if (GET_MODE (op2) != mode)
op2 = gen_lowpart (mode, op2);
tmp2 = ix86_gen_bcst_mem (mode, op2);
if (!tmp2)
{
tmp2 = validize_mem (force
>
> > + if (rtx_equal_p (op0, op1) && rtx_equal_p (op0, op2))
> > + switch (idx & 0x81)
> > + {
> > + case 0x00:
> > + idx = 0x00;
> > + break;
> > + case 0x01:
> > + idx = 0x0f;
> > + break;
> > + case 0x80:
> > + idx = 0xf0;
> > + break;
> > + case 0x81:
> > + idx = 0xff;
> > + break;
> > + }
> > +
> > + switch (idx & 0xff)
> > + {
> > + case 0x00:
> > + if ((!op0 || !side_effects_p (op0))
> > + && (!op1 || !side_effects_p (op1))
> > + && (!op2 || !side_effects_p (op2)))
> > + {
> > + emit_move_insn (target, CONST0_RTX (mode));
> > + return target;
> > + }
> > + break;
> > +
> > + case 0x0a: /* ~a&c */
>
> With the enum idea above, this could be 'case ~TERNLOG_A & TERNLOG_C', etc.
>
> Alexander
--
BR,
Hongtao
