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 <[email protected]> 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
_______________________________________________ iovisor-dev mailing list [email protected] https://lists.iovisor.org/mailman/listinfo/iovisor-dev
