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.
> 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.
And because we also introduced hi32 rand which should only happen if
insn_aux.zext_dst is false. But when zext_dst is false, there are two
situations, one is this insn define a sub-register whose hi32 is not used
later, the other is this insn define a full 64-bit register. hi32 rand
should only happen on the prior situation. So is_reg64 also called to rule
out the latter. The use of is_64 could be removed by changing aux.zext_dst
from bool to enum, so we rename it to aux.reg_def, its value could be:
REG_DEF_NONE (some insn doesn't define value, this is default)
REG_DEF64
REG_DEF32
REG_DEF32_ZEXT
When calling check_reg_arg, and DST/DST_32 will initialize aux.reg_def into
REG_DEF64 and REG_DEF32 accordingly. Then, a later 64-bit use on sub-register
could promote REG_DEF32 into REG_DEF32_ZEXT.
In all, my propose changes are:
1. split enum reg_arg_type, adding new SRC32_OP and DST32_OP, during insn
walking, let call sites of check_reg_arg passing correct type
directly, remove insn re-parsing inside check_reg_arg.
2. keep "is_reg64", it will be used by parsing patched-insn.
3. change aux.zext_dst to aux.reg_def, and change the type from bool to
the enum listed above. When promoting one reg to REG_DEF32_ZEXT, also
do sanity check, the promotion should only happen on REG_DEF32.
Does this looks good?
Regards,
Jiong
