[PATCH] D37262: The issues with X86 prefixes: step 2

[Andrew Tischenko via llvm-commits]

OK, I'll try to change the assembler properly but there are some questions:

 1. Should I do it in the same patch?
 2. Currently if we have:

repz repnz repe cmpsb

     then we produce with 'llvm-mc -triple x86_64-unknown-unknown 
-x86-asm-syntax=intel -show-encoding intel-syntax.s':

         rep                             # encoding: [0xf3]
         repne                           # encoding: [0xf2]
         rep                             # encoding: [0xf3]
         cmpsb   %es:(%rdi), (%rsi)      # encoding: [0xa6]

     but after the change we'll get only the following:

         rep                             # encoding: [0xf3]
         cmpsb   %es:(%rdi), (%rsi)      # encoding: [0xa6]

     Is it OK? (IMHO, Yes.)

     If YES, do we need any warnings here? (IMHO, No.)

Andrew

On 26.09.2017 22:31, Rafael Avila de Espindola wrote:
> The assembler and disassembler should use the same path.
>
> I would be OK with always producing 1 or N instructions, as long as both
> the assembler and disassembler do the same. That is, it is OK to have
> Flags, as long as the assembler uses that instead of creating a separate
> instruction for prefixes.
>
> It seems that allowing the disassembler to create multiple instructions
> would have the advantage of not needing Flags, but that is secondary
> IMHO.
>
> Cheers,
> Rafael
>
> Craig Topper <craig.topper at gmail.com> writes:
>
>> Here's my understanding of what I think happens today.
>>
>> -For a very select few instructions if the AsmParser sees a repne/repe
>> prefix it creates a special version of the instruction that has the REP
>> bits set in TSFlags. For any other instruction it emits the repne/rep/repe
>> as a separate MCInst.
>> -For the disassembler if it sees a repne/repe byte at the start that it
>> doesn't think goes with an instruction it will emit a MCInst containing
>> just the REP.
>> -If the disassembler encounters a repne/repe byte not at the start of the
>> instruction that doesn't go with the instruction we drop it and don't print
>> anything. The disassembler interface only allows us to return one
>> instruction. So we can't return a separate repne/repe instruction and a
>> real instruction from the same byte sequence. I don't believe the assembler
>> can ever produce a byte sequence that hits this case, but that doesn't mean
>> some binary couldn't contain that string of bytes created by hand. So this
>> patch is trying to preserve the extra prefix information in the one MCInst
>> we're allowed to emit. Maybe another option would be to allow creating
>> multiple MCInsts from the disassembler?
>>
>> ~Craig
>>
>> On Tue, Sep 26, 2017 at 10:37 AM, Rafael Avila de Espindola <
>> rafael.espindola at gmail.com> wrote:
>>
>>> The question is why it is different for disassembler than for the
>>> assembler?
>>>
>>> How does the assembler handle trepne?
>>>
>>> Cheers,
>>> Rafael
>>>
>>> Andrew Tischenko <tishenandr at xenzu.com> writes:
>>>
>>>> It is not a simple flag, it's some data. And this data could be useful
>>>> for any other component because it's some opaque info which could be
>>>> send via MCInst from one low level target component to another one.
>>>> Without this (additional) data MCInst loosing (potentially very useful)
>>>> info about the given instruction.
>>>>
>>>> Andrew
>>>>
>>>> On 25.09.2017 22:05, Rafael Avila de Espindola wrote:
>>>>> Having a flag field that is used only on disassembly seems wrong.
>>>>>
>>>>> Don't we support parsing our own output? I don't see trepne in any .s
>>>>> test for example.
>>>>>
>>>>> Cheers,
>>>>> Rafael
>>>>>
>>>>> Craig Topper via Phabricator <reviews at reviews.llvm.org> writes:
>>>>>
>>>>>> craig.topper added a comment.
>>>>>>
>>>>>> I'm not sure I can approve growing the size of MCInst. Though I can't
>>> see anyway around it. @rafael what do you think?
>>>>>>
>>>>>> https://reviews.llvm.org/D37262

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170927/5e92a1d7/attachment.html>