On Thu, Oct 29, 2020 at 7:52 AM Hongyu Wang <wwwhhhyyy...@gmail.com> wrote:
>
> Hi Uros,
>
> > is there a reason to introduce all these (with corresponding changes)?
> > SSE options live in ISA bitmap, so it is kind of strange you need to
> > handle them in ISA2 bitmap. Option handling is not exactly my area,
> > please ask HJ to comment and review this part.
>
> As Hongtao said, this part is needed for new non-avx512 ISAs in ISA2 bitmap.
> Keylocker is the first one, and we do similar thing in AVX-VNNI.
Thanks for the explanation, LGTM for this part.
> > + pat = gen_rtx_EQ (QImode, gen_rtx_REG (CCZmode, FLAGS_REG),
> > + const0_rtx);
> > + emit_move_insn (target, pat);
> >
> > emit_insn (gen_rtx_SET (target, pat));
> >
>
> Changed.
>
> > + op1 = copy_to_suggested_reg (op1,
> > + gen_rtx_REG (V2DImode,
> > + GET_SSE_REGNO (0)),
> > + V2DImode);
> > +
> > + xmm_regs[0] = op1;
> >
> > this is no better than:
> >
> > reg = gen_rtx_REG (V2DImode, GET_SSE_REGNO (0));
> > emit_move_insn (reg, op1)
>
> Changed.
>
> > + xmm_regs[0] = op1;
> > + for (i = 1; i < 3; i++)
> > + xmm_regs[i] = gen_rtx_REG (V2DImode, GET_SSE_REGNO (i));
> >
> > The first line is dead code, copy_to_suggested reg generated (reg
> > xmm0) RTX for op1. Just use:
> >
> > for (i = 0; i < 3; i++)
> > xmm_regs[i] = gen_rtx_REG (V2DImode, GET_SSE_REGNO (i));
> >
> > Similar comments for:
>
> All changed.
>
> > + for (i = 0; i < 4; i++)
> > + {
> > + tmp_unspec
> > + = gen_rtx_UNSPEC_VOLATILE (V2DImode,
> > + gen_rtvec (1, const0_rtx),
> > + UNSPECV_ENCODEKEY256U32);
> >
> > Please move the above out of the loop.
> Sorry for such code. Changed.
>
> > Here lies the reason to use hard registers. "sse_reg_operand" is not
> > enough for register allocator. We have no constraint for first SSE
> > reg, so again
> > use hard registers here instead of operands 2 and 3: (reg:V2DI
> > XMM0_REG) and (reg:V2DI XMM1_REG).
>
> Changed for encodekey128u32, encodekey256u32 expander and
> patterns, adjust corresponding function call in i386-expand.c.
>
> Thanks for all the the helpful comments. Updated patch.
The patch is OK for mainline.
(There are some cleanup opportunities, I'll commit them as a
no-functional-change patch in stage3.)
Thanks,
Uros.
