| Message ID | 20170321001607.75169-4-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Mon, Mar 20, 2017 at 5:15 PM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > +static int compare_swap(int opcode) This function can be call "swap_compare", the action part is swap. > +{ > + switch (opcode) { > + case OP_SET_EQ: return OP_SET_EQ; > + case OP_SET_NE: return OP_SET_NE; > + > + case OP_SET_LT: return OP_SET_GT; > + case OP_SET_LE: return OP_SET_GE; > + case OP_SET_GT: return OP_SET_LT; > + case OP_SET_GE: return OP_SET_LE; > + > + case OP_SET_A: return OP_SET_B; > + case OP_SET_AE: return OP_SET_BE; > + case OP_SET_B: return OP_SET_A; > + case OP_SET_BE: return OP_SET_AE; Some very minor micro optimization note. I take a look at the machine code it generated. It actually generate a jump table and each jump table has some label entry corresponding to code fragment like: movl $28, %eax #, _163 .LVL985: jmp .L701 # I think it should be better to use a static array to directly fetch the new opcode. There is OP_BINCMP and OP_BINCMP_END to mark the begin and end of the binary compare opcode. That way there one data table not jump table for control flow. This comment apply to comparere_opcode() which introduce in the earlier patch as well. Chris -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 23, 2017 at 10:12:28PM -0700, Christopher Li wrote: > On Mon, Mar 20, 2017 at 5:15 PM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > > Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> > > > > +static int compare_swap(int opcode) > > This function can be call "swap_compare", the action part is > swap. Yes, indeed. In fact, I think it need a name which explain even more the purpose. I'll see. > > +{ > > + switch (opcode) { > > + case OP_SET_EQ: return OP_SET_EQ; > > + case OP_SET_NE: return OP_SET_NE; > > + > > + case OP_SET_LT: return OP_SET_GT; > > + case OP_SET_LE: return OP_SET_GE; > > + case OP_SET_GT: return OP_SET_LT; > > + case OP_SET_GE: return OP_SET_LE; > > + > > + case OP_SET_A: return OP_SET_B; > > + case OP_SET_AE: return OP_SET_BE; > > + case OP_SET_B: return OP_SET_A; > > + case OP_SET_BE: return OP_SET_AE; > > Some very minor micro optimization note. > I take a look at the machine code it generated. It actually generate > a jump table and each jump table has some label entry corresponding > to code fragment like: > > movl $28, %eax #, _163 > .LVL985: > jmp .L701 # > > I think it should be better to use a static array to directly > fetch the new opcode. There is OP_BINCMP and OP_BINCMP_END > to mark the begin and end of the binary compare opcode. That > way there one data table not jump table for control flow. > > This comment apply to comparere_opcode() which introduce in the > earlier patch as well. Yes, it's kinda ugly. OTOH, given that jumps are cheaper and cheaper on modern CPU and memory accesses slower and slower, is what you propose really an optimization? And even if it would, does it matter? My preference, if performance mattered here, would be to play with the bits of the opcode, something like: #define OP_SET__EQUAL (1 << ...) #define OP_SET__SIGNED (1 << ...) #define OP_SET__LTHAN ... #define OP_SET__GTHAN ... ... int swap_compare_opcode(opcode) { return opcode ^= (OP_SET__LTHAN|OP_SET__GTHAN); } I'll look if I can get something simple and clean. -- Luc -- To unsubscribe from this list: send the line "unsubscribe linux-sparse" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/simplify.c b/simplify.c index 66035bbce..da40caa65 100644 --- a/simplify.c +++ b/simplify.c @@ -427,6 +427,26 @@ static int compare_opcode(int opcode, int inverse) } } +static int compare_swap(int opcode) +{ + switch (opcode) { + case OP_SET_EQ: return OP_SET_EQ; + case OP_SET_NE: return OP_SET_NE; + + case OP_SET_LT: return OP_SET_GT; + case OP_SET_LE: return OP_SET_GE; + case OP_SET_GT: return OP_SET_LT; + case OP_SET_GE: return OP_SET_LE; + + case OP_SET_A: return OP_SET_B; + case OP_SET_AE: return OP_SET_BE; + case OP_SET_B: return OP_SET_A; + case OP_SET_BE: return OP_SET_AE; + default: + return opcode; + } +} + static int simplify_seteq_setne(struct instruction *insn, long long value) { pseudo_t old = insn->src1; @@ -744,6 +764,14 @@ static int canonicalize_commutative(struct instruction *insn) return repeat_phase |= REPEAT_CSE; } +static int canonicalize_compare(struct instruction *insn) +{ + int repeat = canonicalize_commutative(insn); + if (repeat) + insn->opcode = compare_swap(insn->opcode); + return repeat; +} + static inline int simple_pseudo(pseudo_t pseudo) { return pseudo->type == PSEUDO_VAL || pseudo->type == PSEUDO_SYM; @@ -1139,15 +1167,17 @@ int simplify_instruction(struct instruction *insn) canonicalize_commutative(insn); return simplify_binop(insn); + case OP_SET_LE: case OP_SET_GE: + case OP_SET_LT: case OP_SET_GT: + case OP_SET_B: case OP_SET_A: + case OP_SET_BE: case OP_SET_AE: + canonicalize_compare(insn); + /* fall through */ case OP_SUB: case OP_DIVU: case OP_DIVS: case OP_MODU: case OP_MODS: case OP_SHL: case OP_LSR: case OP_ASR: - case OP_SET_LE: case OP_SET_GE: - case OP_SET_LT: case OP_SET_GT: - case OP_SET_B: case OP_SET_A: - case OP_SET_BE: case OP_SET_AE: return simplify_binop(insn); case OP_NOT: case OP_NEG:
Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- simplify.c | 38 ++++++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-)