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@xxxxxxxxx> 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@xxxxxxxxxxxxx> > 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 >
|