| Message ID | 20170327173405.11405-3-luc.vanoostenryck@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
On Tue, Mar 28, 2017 at 1:33 AM, Luc Van Oostenryck <luc.vanoostenryck@gmail.com> wrote: > + > +const struct opcode_table opcode_table[OP_LAST] = { > + [OP_SET_EQ] = { .negate = OP_SET_NE, }, > + [OP_SET_NE] = { .negate = OP_SET_EQ, }, > + [OP_SET_LT] = { .negate = OP_SET_GE, }, > + [OP_SET_LE] = { .negate = OP_SET_GT, }, > + [OP_SET_GE] = { .negate = OP_SET_LT, }, > + [OP_SET_GT] = { .negate = OP_SET_LE, }, > + [OP_SET_B ] = { .negate = OP_SET_AE, }, > + [OP_SET_BE] = { .negate = OP_SET_A , }, > + [OP_SET_AE] = { .negate = OP_SET_B , }, > + [OP_SET_A ] = { .negate = OP_SET_BE, }, > +}; The .negate member is only used by simplify_seteq_setne() Also the .swap member is only used by canonicalize_compare() in the later patch. Why not make it just two array local to those function who use it? I am not sure this need to be a global data structure. This is very minor, I can also accept it as it is. Also I would like to point out, at the code that use it. In a later patch. + insn->opcode = opcode_table[insn->opcode].swap; That is a different behavior than the V4 patch. In the V4 patch if your input opcode is not in the compare group, the opcode is not changed. Here .negate and .swap will get to zero as opcode. You might want to check for that. 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 Fri, Mar 31, 2017 at 12:30 PM, Christopher Li <sparse@chrisli.org> wrote: > On Tue, Mar 28, 2017 at 1:33 AM, Luc Van Oostenryck > <luc.vanoostenryck@gmail.com> wrote: > >> + >> +const struct opcode_table opcode_table[OP_LAST] = { >> + [OP_SET_EQ] = { .negate = OP_SET_NE, }, >> + [OP_SET_NE] = { .negate = OP_SET_EQ, }, >> + [OP_SET_LT] = { .negate = OP_SET_GE, }, >> + [OP_SET_LE] = { .negate = OP_SET_GT, }, >> + [OP_SET_GE] = { .negate = OP_SET_LT, }, >> + [OP_SET_GT] = { .negate = OP_SET_LE, }, >> + [OP_SET_B ] = { .negate = OP_SET_AE, }, >> + [OP_SET_BE] = { .negate = OP_SET_A , }, >> + [OP_SET_AE] = { .negate = OP_SET_B , }, >> + [OP_SET_A ] = { .negate = OP_SET_BE, }, >> +}; > > The .negate member is only used by simplify_seteq_setne() > Also the .swap member is only used by canonicalize_compare() > in the later patch. Why not make it just two array local to those function > who use it? I am not sure this need to be a global data structure. > This is very minor, I can also accept it as it is. I know, I have used a local table for the negate, then I needed one for the swap and in the next series I needed another one for "convert to float" relation. At that point it made more sense to use a single table for all of them. The .swap part is extended in the next series and the .negate part will also be used in coming patches (some already written but not yet published, some planned but not yet really written). I think that things are clearer and cleaner like this. I prefer to leave it as is for the moment. (I realize that I'm not very receptive to small changes, sorry for that, but I would like to focus more on a few big, fundamental issues). > Also I would like to point out, at the code that use it. In a later patch. > + insn->opcode = opcode_table[insn->opcode].swap; > > That is a different behavior than the V4 patch. In the V4 patch > if your input opcode is not in the compare group, the opcode > is not changed. Here .negate and .swap will get to zero as opcode. > You might want to check for that. Yes, I've reorganised things a little bit but it's fine as it's only called when it is known that the input opcode is one for which there must be a valid entry in the table (in this case called in canonicalize_compare() which itself is only called for OP_SET_LT, OP_SET_....) -- 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/Makefile b/Makefile index 76902b75e..3ac100744 100644 --- a/Makefile +++ b/Makefile @@ -106,6 +106,7 @@ LIB_OBJS= target.o parse.o tokenize.o pre-process.o symbol.o lib.o scope.o \ expression.o show-parse.o evaluate.o expand.o inline.o linearize.o \ char.o sort.o allocate.o compat-$(OS).o ptrlist.o \ builtin.o \ + opcode.o \ flow.o cse.o simplify.o memops.o liveness.o storage.o unssa.o dissect.o LIB_FILE= libsparse.a diff --git a/linearize.h b/linearize.h index bac82d7ff..5ae78f596 100644 --- a/linearize.h +++ b/linearize.h @@ -4,6 +4,7 @@ #include "lib.h" #include "allocate.h" #include "token.h" +#include "opcode.h" #include "parse.h" #include "symbol.h" @@ -217,6 +218,8 @@ enum opcode { /* Needed to translate SSA back to normal form */ OP_COPY, + + OP_LAST, /* keep this one last! */ }; struct basic_block_list; diff --git a/opcode.c b/opcode.c new file mode 100644 index 000000000..0aed1ca1f --- /dev/null +++ b/opcode.c @@ -0,0 +1,36 @@ +/* + * Copyright (C) 2017 Luc Van Oostenryck + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +#include "linearize.h" + +const struct opcode_table opcode_table[OP_LAST] = { + [OP_SET_EQ] = { .negate = OP_SET_NE, }, + [OP_SET_NE] = { .negate = OP_SET_EQ, }, + [OP_SET_LT] = { .negate = OP_SET_GE, }, + [OP_SET_LE] = { .negate = OP_SET_GT, }, + [OP_SET_GE] = { .negate = OP_SET_LT, }, + [OP_SET_GT] = { .negate = OP_SET_LE, }, + [OP_SET_B ] = { .negate = OP_SET_AE, }, + [OP_SET_BE] = { .negate = OP_SET_A , }, + [OP_SET_AE] = { .negate = OP_SET_B , }, + [OP_SET_A ] = { .negate = OP_SET_BE, }, +}; diff --git a/opcode.h b/opcode.h new file mode 100644 index 000000000..3a89de05e --- /dev/null +++ b/opcode.h @@ -0,0 +1,9 @@ +#ifndef OPCODE_H +#define OPCODE_H + + +extern const struct opcode_table { + int negate:8; +} opcode_table[]; + +#endif
Some optimizations transform an instruction opcode into another. For example, it may be needed to know the opcode corresponding to the negation of a comparison. This patch make this easy and flexible by adding a table for the relation between opcodes. Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com> --- Makefile | 1 + linearize.h | 3 +++ opcode.c | 36 ++++++++++++++++++++++++++++++++++++ opcode.h | 9 +++++++++ 4 files changed, 49 insertions(+) create mode 100644 opcode.c create mode 100644 opcode.h