On Thu, May 23, 2019 at 09:20:15PM +0100, Jiong Wang wrote:
>
> Alexei Starovoitov writes:
>
> <snip>
>
> > well, it made me realize that we're probably doing it wrong,
> > since after calling check_reg_arg() we need to re-parse insn encoding.
> > How about we change check_reg_arg()'s enum reg_arg_type instead?
>
> This is exactly what I had implemented in my initial internal version.
it was long ago :) shrinkers purged it.
> > The caller has much more context and no need to parse insn opcode again.
>
> And I had exactly the same thoughts, is_reg64 is duplicating what has been
> done.
>
> The code evolved into the current shape mostly because I agreed if we
> re-centre all checks inside check_reg_arg, then we don't need to touch
> quite a few call sites of check_reg_arg, the change/patch looks simpler,
> and I was thinking is_reg64 is a quick check, so the overhead is not big.
>
> > Something like:
> > enum reg_arg_type {
> > SRC_OP64,
> > DST_OP64,
> > DST_OP_NO_MARK, // probably no need to split this one ?
> > SRC_OP32,
> > DST_OP32,
> > };
> >
>
> Yeah, no need to split DST_OP_NO_MARK, my split was
>
> enum reg_arg_type {
> SRC_OP,
> + SRC32_OP,
> DST_OP,
> + DST32_OP,
> DST_OP_NO_MARK
> }
>
> No renaming on existing SRC/DST_OP, they mean 64-bit, the changes are
> smaller, looks better?
>
> But, we also need to know whether one patch-insn define sub-register, if it
> is, we then conservatively mark it as needing zero extension. patch-insn
> doesn't go through check_reg_arg analysis, so still need separate insn
> parsing.
Good point.
Then let's stick with your last is_reg64().
Re-parsing is annoying, but looks like there is already use case for it
and more can appear in the future.
