[iovisor-dev] [v2][PATCH RFC] Add BPF AsmParser support in LLVM

Y Song ys114321 at gmail.com
Tue Sep 12 18:12:28 UTC 2017


Sorry. Resend with "plain text format" to satisfy xdp-newbies mailing
list requirement.

On Tue, Sep 12, 2017 at 11:10 AM, Y Song <ys114321 at gmail.com> wrote:
> Thanks, Jiong,
>
> The patch looks good. I have applied to llvm trunk.
> https://reviews.llvm.org/rL313055
> A little bit more comments below.
>
> On Tue, Sep 12, 2017 at 4:10 AM, Jiong Wang <jiong.wang at netronome.com>
> wrote:
>>>
>>>
>>>>     - Adjusted constant load tests.
>>>>     - Interleaved the expected results with input insns.
>>>>
>>>>   * Changed the disassembly output "ll" to "LL" in BPFInstPrinter.cpp.
>>>>     This is because I noticed MC parser only accept "LL" as long long
>>>> suffix.
>>>
>>>
>>> Sorry for the confusion caused by "ll". I tested your patch and found
>>> that
>>> both r = 5 and r = 5LL generated ld_imm64 insn. This is not what compiler
>>> will do. Further debugging I found that this is due to my change to
>>> remove
>>> "ll" from asmstring in the insn definition (.td file). Since the
>>> assembler is
>>> patten matching based on asmstring. "r = 5" matched to ld_imm64 as well.
>>>
>>> I just fixed the issue and restored " ll" suffix for ld_imm64 asmstring.
>>> You can then restore your previous change to add "ll" as a valid
>>> identifier
>>> in the middle of asm statement.
>>
>>
>> Hmm, I actually feel there is no need to offer an separate syntax for
>> 64bit imm
>> encoding, it might be better to offer a unified single syntax "r =
>> signed_imm"
>> to the user and the encoder (BPFMCCodeEmitter::encodeInstruction)
>> guarantee to
>> choose optimal encoding.
>>
>> Encoder could choose BPF_ALU64 | BPF_MOV | BPF_K for both "r1 = -1" and
>> "r1 = -1ll" while only resorting to BPF_LD | BPF_DW | BPF_IMM when the imm
>> really
>> doesn't fit into 32bit (!isInt<32>(imm_opnd)), for example, "r1 =
>> 0x1ffffffffll".
>>
> Right now, this optimization actually
> has been done in compiler side. So depending on the value, the compiler will
> use
> "move r1, <32_bit_value>" or "ld_imm64 r1, <greater_than_32_bit_value>".
> In this case, it make sense to have different insn formats for two different
> kinds of
> insns.
>
> What you described is to move the optimization down to MC Emit Code stage,
> based on
> the value, it can choose to use "move" or "ld_imm64" insn. So optimization
> will be available
> for .c => .o and for .s => .o. (The current scheme, optimization not
> available from .s => .o).
>
> Yes, it is possible. But typically, there is one-to-one mapping between asm
> insn to insn encoding.
> If later on, we found the complain about this, we can revisit this issue.
>
> Thanks.
> Yonghong
>


More information about the iovisor-dev mailing list