| Message ID | 1611113349-24906-13-git-send-email-tsimpson@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,01/35] Hexagon Update MAINTAINERS file | expand |
On 1/20/21 4:28 AM, Taylor Simpson wrote: > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> > --- > target/hexagon/attribs.h | 30 ++++++++++++++ > target/hexagon/attribs_def.h | 95 ++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 125 insertions(+) > create mode 100644 target/hexagon/attribs.h > create mode 100644 target/hexagon/attribs_def.h > > diff --git a/target/hexagon/attribs.h b/target/hexagon/attribs.h > new file mode 100644 > index 0000000..e88e5eb > --- /dev/null > +++ b/target/hexagon/attribs.h > @@ -0,0 +1,30 @@ > +/* > + * Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program; if not, see <http://www.gnu.org/licenses/>. > + */ > + > +#ifndef HEXAGON_ATTRIBS_H > +#define HEXAGON_ATTRIBS_H > + > +enum { > +#define DEF_ATTRIB(NAME, ...) A_##NAME, > +#include "attribs_def.h" Per QEMU conventions, this file has to be named "attribs_def.h.inc". Otherwise: Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> -----Original Message----- > From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On > Behalf Of Philippe Mathieu-Daudé > Sent: Friday, January 22, 2021 11:54 AM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org > Cc: richard.henderson@linaro.org; alex.bennee@linaro.org; > laurent@vivier.eu; ale@rev.ng; Brian Cain <bcain@quicinc.com> > Subject: Re: [PATCH v7 12/35] Hexagon (target/hexagon) instruction > attributes > > On 1/20/21 4:28 AM, Taylor Simpson wrote: > > Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> > > --- > > target/hexagon/attribs.h | 30 ++++++++++++++ > > target/hexagon/attribs_def.h | 95 > ++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 125 insertions(+) > > create mode 100644 target/hexagon/attribs.h > > create mode 100644 target/hexagon/attribs_def.h > > > > diff --git a/target/hexagon/attribs.h b/target/hexagon/attribs.h > > new file mode 100644 > > index 0000000..e88e5eb > > --- /dev/null > > +++ b/target/hexagon/attribs.h > > @@ -0,0 +1,30 @@ > > + > > +enum { > > +#define DEF_ATTRIB(NAME, ...) A_##NAME, > > +#include "attribs_def.h" > > Per QEMU conventions, this file has to be named "attribs_def.h.inc". Didn't know that. Which files should end in .inc? > > Otherwise: > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> Thanks!!
On 1/22/21 11:01 PM, Taylor Simpson wrote: >> -----Original Message----- >> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On >> Behalf Of Philippe Mathieu-Daudé >> Sent: Friday, January 22, 2021 11:54 AM >> To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org >> Cc: richard.henderson@linaro.org; alex.bennee@linaro.org; >> laurent@vivier.eu; ale@rev.ng; Brian Cain <bcain@quicinc.com> >> Subject: Re: [PATCH v7 12/35] Hexagon (target/hexagon) instruction >> attributes >> >> On 1/20/21 4:28 AM, Taylor Simpson wrote: >>> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> >>> --- >>> target/hexagon/attribs.h | 30 ++++++++++++++ >>> target/hexagon/attribs_def.h | 95 >> ++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 125 insertions(+) >>> create mode 100644 target/hexagon/attribs.h >>> create mode 100644 target/hexagon/attribs_def.h >>> >>> diff --git a/target/hexagon/attribs.h b/target/hexagon/attribs.h >>> new file mode 100644 >>> index 0000000..e88e5eb >>> --- /dev/null >>> +++ b/target/hexagon/attribs.h >>> @@ -0,0 +1,30 @@ >>> + >>> +enum { >>> +#define DEF_ATTRIB(NAME, ...) A_##NAME, >>> +#include "attribs_def.h" >> >> Per QEMU conventions, this file has to be named "attribs_def.h.inc". > > Didn't know that. Which files should end in .inc? Oh you are right, it is not documented in CODING_STYLE.rst. You can see the rationale in commits:139c1837db7 and 0979ed017f0: meson: rename included C source files to .c.inc With Makefiles that have automatically generated dependencies, you generated includes are set as dependencies of the Makefile, so that they are built before everything else and they are available when first building the .c files. Alternatively you can use a fine-grained dependency, e.g. target/arm/translate.o: target/arm/decode-neon-shared.inc.c With Meson you have only one choice and it is a third option, namely "build at the beginning of the corresponding target"; the way you express it is to list the includes in the sources of that target. The problem is that Meson decides if something is a source vs. a generated include by looking at the extension: '.c', '.cc', '.m', '.C' are sources, while everything else is considered an include---including '.inc.c'. Use '.c.inc' to avoid this, as it is consistent with our other convention of using '.rst.inc' for included reStructuredText files. The editorconfig file is adjusted. > >> >> Otherwise: >> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Thanks!! > >
> -----Original Message----- > From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On > Behalf Of Philippe Mathieu-Daudé > Sent: Monday, January 25, 2021 10:21 AM > To: Taylor Simpson <tsimpson@quicinc.com>; qemu-devel@nongnu.org; > Paolo Bonzini <pbonzini@redhat.com> > Cc: ale@rev.ng; alex.bennee@linaro.org; richard.henderson@linaro.org; > laurent@vivier.eu; Brian Cain <bcain@quicinc.com> > Subject: Re: [PATCH v7 12/35] Hexagon (target/hexagon) instruction > attributes > > >> On 1/20/21 4:28 AM, Taylor Simpson wrote: > >>> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> > >>> --- > >>> target/hexagon/attribs.h | 30 ++++++++++++++ > >>> target/hexagon/attribs_def.h | 95 > >> ++++++++++++++++++++++++++++++++++++++++++++ > >>> 2 files changed, 125 insertions(+) > >>> create mode 100644 target/hexagon/attribs.h > >>> create mode 100644 target/hexagon/attribs_def.h > >>> > >>> diff --git a/target/hexagon/attribs.h b/target/hexagon/attribs.h > >>> new file mode 100644 > >>> index 0000000..e88e5eb > >>> --- /dev/null > >>> +++ b/target/hexagon/attribs.h > >>> @@ -0,0 +1,30 @@ > >>> + > >>> +enum { > >>> +#define DEF_ATTRIB(NAME, ...) A_##NAME, > >>> +#include "attribs_def.h" > >> > >> Per QEMU conventions, this file has to be named "attribs_def.h.inc". > > > > Didn't know that. Which files should end in .inc? > > Oh you are right, it is not documented in CODING_STYLE.rst. > > You can see the rationale in commits:139c1837db7 and 0979ed017f0: > > meson: rename included C source files to .c.inc > > With Makefiles that have automatically generated dependencies, you > generated includes are set as dependencies of the Makefile, so that they > are built before everything else and they are available when first > building the .c files. > > Alternatively you can use a fine-grained dependency, e.g. > > target/arm/translate.o: target/arm/decode-neon-shared.inc.c > > With Meson you have only one choice and it is a third option, namely > "build at the beginning of the corresponding target"; the way you > express it is to list the includes in the sources of that target. > > The problem is that Meson decides if something is a source vs. a > generated include by looking at the extension: '.c', '.cc', '.m', '.C' > are sources, while everything else is considered an include---including > '.inc.c'. > > Use '.c.inc' to avoid this, as it is consistent with our other convention > of using '.rst.inc' for included reStructuredText files. The editorconfig > file is adjusted. OK, I understand why it's better to have files end .[ch].inc than .inc.[ch]. However, I need some confirmation on which files need .inc instead of simply ending in .h. From what I can tell these are the guidelines - If a file is intended to be included in the middle of another file (as opposed to the top), it should end in .inc. - If a .inc file is intended to be included in a .h file, it should end in .h.inc. - If a .inc file is intended to be included in a .c file, it should end in .c.inc. - The above applies to both human-written and generated files. Thanks, Taylor
Hi Taylor, +Eric in case I'm wrong. On 1/30/21 12:15 AM, Taylor Simpson wrote: >>>> On 1/20/21 4:28 AM, Taylor Simpson wrote: >>>>> Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> >>>>> --- >>>>> target/hexagon/attribs.h | 30 ++++++++++++++ >>>>> target/hexagon/attribs_def.h | 95 >>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>>> 2 files changed, 125 insertions(+) >>>>> create mode 100644 target/hexagon/attribs.h >>>>> create mode 100644 target/hexagon/attribs_def.h >>>>> >>>>> diff --git a/target/hexagon/attribs.h b/target/hexagon/attribs.h >>>>> new file mode 100644 >>>>> index 0000000..e88e5eb >>>>> --- /dev/null >>>>> +++ b/target/hexagon/attribs.h >>>>> @@ -0,0 +1,30 @@ >>>>> + >>>>> +enum { >>>>> +#define DEF_ATTRIB(NAME, ...) A_##NAME, >>>>> +#include "attribs_def.h" >>>> >>>> Per QEMU conventions, this file has to be named "attribs_def.h.inc". >>> >>> Didn't know that. Which files should end in .inc? >> >> Oh you are right, it is not documented in CODING_STYLE.rst. >> >> You can see the rationale in commits:139c1837db7 and 0979ed017f0: >> >> meson: rename included C source files to .c.inc >> >> With Makefiles that have automatically generated dependencies, you >> generated includes are set as dependencies of the Makefile, so that they >> are built before everything else and they are available when first >> building the .c files. >> >> Alternatively you can use a fine-grained dependency, e.g. >> >> target/arm/translate.o: target/arm/decode-neon-shared.inc.c >> >> With Meson you have only one choice and it is a third option, namely >> "build at the beginning of the corresponding target"; the way you >> express it is to list the includes in the sources of that target. >> >> The problem is that Meson decides if something is a source vs. a >> generated include by looking at the extension: '.c', '.cc', '.m', '.C' >> are sources, while everything else is considered an include---including >> '.inc.c'. >> >> Use '.c.inc' to avoid this, as it is consistent with our other convention >> of using '.rst.inc' for included reStructuredText files. The editorconfig >> file is adjusted. > > OK, I understand why it's better to have files end .[ch].inc than .inc.[ch]. > > However, I need some confirmation on which files need .inc instead of simply ending in .h. From what I can tell these are the guidelines > - If a file is intended to be included in the middle of another file (as opposed to the top), it should end in .inc. This has to be justified. Usually such file use macro definitions which are defined by the file including them. If you can not justify, there is probably a way to have your file as its own .c/.h unit. > - If a .inc file is intended to be included in a .h file, it should end in .h.inc. Yes, no exception. > - If a .inc file is intended to be included in a .c file, it should end in .c.inc. Not necessarily, you can have .h.inc included in .c.inc: Function prototype declarations -> .h If generated -> .h.inc Function body definitions -> .c These can NOT go in .h, so if generated -> .c.inc Inlined function body definitions -> .h/.c/.h.inc > - The above applies to both human-written and generated files. Yes, although it is harder to justify human-written .inc. Also: Header exposing subsystem X API to other subsystems go in include/..X..h (example include/hw/sd/sd.h) Header sharing prototypes local to a particular subsystem go in X/..h (example hw/sd/sdmmc-internal.h) *.inc must not go in include/ Regards (and sorry for answering late), Phil.
diff --git a/target/hexagon/attribs.h b/target/hexagon/attribs.h new file mode 100644 index 0000000..e88e5eb --- /dev/null +++ b/target/hexagon/attribs.h @@ -0,0 +1,30 @@ +/* + * Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#ifndef HEXAGON_ATTRIBS_H +#define HEXAGON_ATTRIBS_H + +enum { +#define DEF_ATTRIB(NAME, ...) A_##NAME, +#include "attribs_def.h" +#undef DEF_ATTRIB +}; + +#define GET_ATTRIB(opcode, attrib) \ + test_bit(attrib, opcode_attribs[opcode]) + +#endif /* ATTRIBS_H */ diff --git a/target/hexagon/attribs_def.h b/target/hexagon/attribs_def.h new file mode 100644 index 0000000..f4fcd20 --- /dev/null +++ b/target/hexagon/attribs_def.h @@ -0,0 +1,95 @@ +/* + * Copyright(c) 2019-2021 Qualcomm Innovation Center, Inc. All Rights Reserved. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +/* Keep this as the first attribute: */ +DEF_ATTRIB(AA_DUMMY, "Dummy Zeroth Attribute", "", "") + +/* Misc */ +DEF_ATTRIB(EXTENSION, "Extension instruction", "", "") + +DEF_ATTRIB(PRIV, "Not available in user or guest mode", "", "") +DEF_ATTRIB(GUEST, "Not available in user mode", "", "") + +DEF_ATTRIB(FPOP, "Floating Point Operation", "", "") + +DEF_ATTRIB(EXTENDABLE, "Immediate may be extended", "", "") + +DEF_ATTRIB(ARCHV2, "V2 architecture", "", "") +DEF_ATTRIB(ARCHV3, "V3 architecture", "", "") +DEF_ATTRIB(ARCHV4, "V4 architecture", "", "") +DEF_ATTRIB(ARCHV5, "V5 architecture", "", "") + +DEF_ATTRIB(SUBINSN, "sub-instruction", "", "") + +/* Load and Store attributes */ +DEF_ATTRIB(LOAD, "Loads from memory", "", "") +DEF_ATTRIB(STORE, "Stores to memory", "", "") +DEF_ATTRIB(MEMLIKE, "Memory-like instruction", "", "") +DEF_ATTRIB(MEMLIKE_PACKET_RULES, "follows Memory-like packet rules", "", "") + + +/* Change-of-flow attributes */ +DEF_ATTRIB(JUMP, "Jump-type instruction", "", "") +DEF_ATTRIB(INDIRECT, "Absolute register jump", "", "") +DEF_ATTRIB(CALL, "Function call instruction", "", "") +DEF_ATTRIB(COF, "Change-of-flow instruction", "", "") +DEF_ATTRIB(CONDEXEC, "May be cancelled by a predicate", "", "") +DEF_ATTRIB(DOTNEWVALUE, "Uses a register value generated in this pkt", "", "") +DEF_ATTRIB(NEWCMPJUMP, "Compound compare and jump", "", "") + +/* access to implicit registers */ +DEF_ATTRIB(IMPLICIT_WRITES_LR, "Writes the link register", "", "UREG.LR") +DEF_ATTRIB(IMPLICIT_WRITES_SP, "Writes the stack pointer", "", "UREG.SP") +DEF_ATTRIB(IMPLICIT_WRITES_FP, "Writes the frame pointer", "", "UREG.FP") +DEF_ATTRIB(IMPLICIT_WRITES_LC0, "Writes loop count for loop 0", "", "UREG.LC0") +DEF_ATTRIB(IMPLICIT_WRITES_LC1, "Writes loop count for loop 1", "", "UREG.LC1") +DEF_ATTRIB(IMPLICIT_WRITES_SA0, "Writes start addr for loop 0", "", "UREG.SA0") +DEF_ATTRIB(IMPLICIT_WRITES_SA1, "Writes start addr for loop 1", "", "UREG.SA1") +DEF_ATTRIB(IMPLICIT_WRITES_P0, "Writes Predicate 0", "", "UREG.P0") +DEF_ATTRIB(IMPLICIT_WRITES_P1, "Writes Predicate 1", "", "UREG.P1") +DEF_ATTRIB(IMPLICIT_WRITES_P2, "Writes Predicate 1", "", "UREG.P2") +DEF_ATTRIB(IMPLICIT_WRITES_P3, "May write Predicate 3", "", "UREG.P3") + +DEF_ATTRIB(CRSLOT23, "Can execute in slot 2 or slot 3 (CR)", "", "") +DEF_ATTRIB(IT_NOP, "nop instruction", "", "") +DEF_ATTRIB(IT_EXTENDER, "constant extender instruction", "", "") + + +/* Restrictions to make note of */ +DEF_ATTRIB(RESTRICT_SLOT0ONLY, "Must execute on slot0", "", "") +DEF_ATTRIB(RESTRICT_SLOT1ONLY, "Must execute on slot1", "", "") +DEF_ATTRIB(RESTRICT_SLOT2ONLY, "Must execute on slot2", "", "") +DEF_ATTRIB(RESTRICT_SLOT3ONLY, "Must execute on slot3", "", "") +DEF_ATTRIB(RESTRICT_NOSLOT1, "No slot 1 instruction in parallel", "", "") +DEF_ATTRIB(RESTRICT_PREFERSLOT0, "Try to encode into slot 0", "", "") + +DEF_ATTRIB(ICOP, "Instruction cache op", "", "") + +DEF_ATTRIB(HWLOOP0_END, "Ends HW loop0", "", "") +DEF_ATTRIB(HWLOOP1_END, "Ends HW loop1", "", "") +DEF_ATTRIB(DCZEROA, "dczeroa type", "", "") +DEF_ATTRIB(ICFLUSHOP, "icflush op type", "", "") +DEF_ATTRIB(DCFLUSHOP, "dcflush op type", "", "") +DEF_ATTRIB(DCFETCH, "dcfetch type", "", "") + +DEF_ATTRIB(L2FETCH, "Instruction is l2fetch type", "", "") + +DEF_ATTRIB(ICINVA, "icinva", "", "") +DEF_ATTRIB(DCCLEANINVA, "dccleaninva", "", "") + +/* Keep this as the last attribute: */ +DEF_ATTRIB(ZZ_LASTATTRIB, "Last attribute in the file", "", "")
Signed-off-by: Taylor Simpson <tsimpson@quicinc.com> --- target/hexagon/attribs.h | 30 ++++++++++++++ target/hexagon/attribs_def.h | 95 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 target/hexagon/attribs.h create mode 100644 target/hexagon/attribs_def.h