Re: [PATCH 02/12] x86: fold various AVX512VL templates into their AVX512F counterparts

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Mon, Jul 16, 2018 at 5:59 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 16.07.18 at 14:45, <hjl.tools@gmail.com> wrote:
>> On Wed, Jul 11, 2018 at 2:30 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 04.06.18 at 15:19, <hjl.tools@gmail.com> wrote:
>>>> On Mon, Jun 4, 2018 at 6:03 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> If you dislike the shorthand approach, passing i386-opc.tbl through
>>>>> the C pre-processor would be another option.
>>>>
>>>> I prefer a C pre-processor approach.
>>>
>>> Before spamming the list with the entire updated series, here's the
>>> half way reasonable patch I've been able to come up with (I continue
>>> to prefer the original approach though). If that's acceptable, I can
>>> submit the full series. If you dislike any of it, please give concrete
>>> suggestions on what to change.
>>>
>>> What I specifically don't fancy doing is anything that would require
>>> me to truly re-generate Makefile.in (with the "correct" version of
>>> automake). Should that be required for any reason, I'd then like to
>>
>> I do regenerate Makefile.in and I have to do the same.  It shouldn't
>> be the reason not to use a C compiler.
>
> This isn't about using a C compiler, but about possibly having to do
> more fancy automake-ery in Makefile.ac. If the patch as presented
> is fine, then all is good.
>
>>> propose that I revert back to the original approach, allowing you
>>> to convert to pre-processing if that suits you better.
>>>
>>> One of the fundamental reasons why I'd prefer to stick to the first
>>> version's approach is that there's nowhere in binutils that I could
>>> find that pre-processing as a solitary step was used for anything.
>>> I'm simply concerned that with non-gcc compilers things may not
>>> work as expected.
>>
>> There is only one i386-gen.c and nothing else.  We can add
>> pre-processing if needed.
>
> I don't think I understand this remark.

I thought you were looking for precedent and I don't think it should
be an issue.

>>> --- a/opcodes/Makefile.am
>>> +++ b/opcodes/Makefile.am
>>> @@ -544,8 +544,10 @@ i386-gen.o: i386-gen.c i386-opc.h $(srcd
>>>  $(srcdir)/i386-tbl.h: $(srcdir)/i386-init.h
>>>         @echo $@
>>>
>>> -$(srcdir)/i386-init.h: @MAINT@ i386-gen$(EXEEXT_FOR_BUILD) i386-opc.tbl i386-reg.tbl
>>> -       ./i386-gen$(EXEEXT_FOR_BUILD) --srcdir $(srcdir)
>>> +$(srcdir)/i386-init.h: @MAINT@ i386-gen$(EXEEXT_FOR_BUILD) i386-opc.tbl i386-reg.tbl i386-opc.h
>>> +       $(CPP) $(DEFS) $(DEFAULT_INCLUDES) $(INCLUDES) $(AM_CPPFLAGS) $(CPPFLAGS) - \
>>> +               < $(srcdir)/i386-opc.tbl \
>>> +               | ./i386-gen$(EXEEXT_FOR_BUILD) --srcdir $(srcdir)
>>>
>>>  i386-opc.lo: $(srcdir)/i386-tbl.h
>>>
>>> --- a/opcodes/i386-gen.c
>>> +++ b/opcodes/i386-gen.c
>>> @@ -1262,14 +1262,10 @@ process_i386_opcodes (FILE *table)
>>>    htab_t opcode_hash_table;
>>>    struct opcode_hash_entry **opcode_array;
>>>    unsigned int opcode_array_size = 1024;
>>> -  int lineno = 0;
>>> +  int lineno = 0, marker = 0;
>>>
>>>    filename = "i386-opc.tbl";
>>> -  fp = fopen (filename, "r");
>>> -
>>> -  if (fp == NULL)
>>> -    fail (_("can't find i386-opc.tbl for reading, errno = %s\n"),
>>> -         xstrerror (errno));
>>> +  fp = stdin;
>>>
>>>    i = 0;
>>>    opcode_array = (struct opcode_hash_entry **)
>>> @@ -1303,11 +1299,32 @@ process_i386_opcodes (FILE *table)
>>>        switch (p[0])
>>>         {
>>>         case '#':
>>> +         if (!strcmp("### MARKER ###", buf))
>>> +           marker = 1;
>>> +         else
>>> +           {
>>> +             /* Since we ignore all included files (we only care about their
>>> +                #define-s here), we don't need to monitor filenames.  The final
>>> +                line number directive is going to refer to the main source file
>>> +                again.  */
>>> +             char *end;
>>> +             unsigned long ln;
>>> +
>>> +             p = remove_leading_whitespaces (p + 1);
>>> +             if (!strncmp(p, "line", 4))
>>> +               p += 4;
>>> +             ln = strtoul (p, &end, 10);
>>> +             if (ln > 1 && ln < INT_MAX
>>> +                 && *remove_leading_whitespaces (end) == '"')
>>> +               lineno = ln - 1;
>>> +           }
>>>           /* Ignore comments.  */
>>>         case '\0':
>>>           continue;
>>>           break;
>>>         default:
>>> +         if (!marker)
>>> +           continue;
>>>           break;
>>>         }
>>>
>>> --- a/opcodes/i386-opc.tbl
>>> +++ b/opcodes/i386-opc.tbl
>>> @@ -18,6 +18,12 @@
>>>  // Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
>>>  // 02110-1301, USA.
>>>
>>> +#define OPCODE_I386_H
>>> +#include "i386-opc.h"
>>> +#undef None
>>> +
>>> +### MARKER ###
>>> +
>>>  // Move instructions.
>>>  // We put the 64bit displacement first and we only mark constants
>>>  // larger than 32bit as Disp64.
>>>
>>>
>>>
>>
>> I need to see the full patch.
>
> This is the full patch (minus the ChangeLog entries of course). All
> following patches are basically or entirely unchanged from this
> submission (the sole exception being the mechanical switch to this
> pre-processing model), just that there are several further patches
> at the end now.
>
> Jan
>

I'd like to see the full patch without

+static const initializer opcode_modifier_shorthands[] =
+{
+  { "Disp8ShiftVL", "Disp8MemShift=" stringify(DISP8_SHIFT_VL) },
+};

-- 
H.J.