eddyz87 added a comment. Hi Yonghong,
Left a few nitpicks and one comment in `BPFMIPreEmitPeephole::adjustBranch()` that I think points to a bug. Overall `adjustBranch()` algorithm looks good. It would be great to have some test cases for it, e.g. preprocess test .ll by replacing some template with a bunch of noops or something like this. The instruction encoding seem to match mailing list description. Also one `check-all` test is failing for me: home/eddy/work/llvm-project/clang/test/Misc/target-invalid-cpu-note.c:76:14: error: BPF-NEXT: expected string not found in input // BPF-NEXT: note: valid target CPU values are: generic, v1, v2, v3, probe{{$}} ================ Comment at: llvm/lib/Target/BPF/BPFInstrFormats.td:93 def BPF_MEM : BPFModeModifer<0x3>; +def BPF_MEMS : BPFModeModifer<0x4>; def BPF_ATOMIC : BPFModeModifer<0x6>; ---------------- Nitpick: the mailing list doc refers to this as `BPF_SMEM`. ================ Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:331 + if (IsCPUv4) + Changed = adjustBranch() || Changed; + return Changed; ---------------- Nitpick: this would not be executed for `-O0`, but is required for correct execution. ``` void BPFPassConfig::addPreEmitPass() { addPass(createBPFMIPreEmitCheckingPass()); if (getOptLevel() != CodeGenOpt::None) if (!DisableMIPeephole) addPass(createBPFMIPreEmitPeepholePass()); } ``` ================ Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:452 + JmpBB = UncondJmp->getOperand(0).getMBB(); + Dist = SoFarNumInsns[JmpBB] - CurrNumInsns; + if (Dist <= INT16_MAX && Dist >= INT16_MIN) ---------------- As far as I understand: - `SoFarNumInsns[JmpBB]` is a number of instructions from function start till the end of `JmpBB`; - `CurrNumInsns` is a number of instructions from function start till the end of `MBB`. So, `SoFarNumInsns[JmpBB] - CurrNumInsns` gives the distance between basic block ends. However, the jump would happen to the basic block start, so the actual distance should be computed as `SoFarNumInsns[JmpBB] - JmpBB.size() - CurrNumInsns`. Am I confused? ================ Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:479 + // + // We do not have 32bit cond jmp insn. So we try to do + // the following. ---------------- Is it possible to rewrite as below instead? ``` B2: ... if (!cond) goto B3 gotol B5 B3: ... ``` Seems to be equivalent but with less instructions. ================ Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:549 + Dist = SoFarNumInsns[CondTargetBB] - CurrNumInsns; + if (Dist > INT16_MAX || Dist < INT16_MIN) { + MachineBasicBlock *New_B = MF->CreateMachineBasicBlock(TermBB); ---------------- Nitpick: `(Dist <= INT16_MAX && Dist >= INT16_MIN)` is used in the previous two cases. ================ Comment at: llvm/lib/Target/BPF/MCTargetDesc/BPFAsmBackend.cpp:108 + } else if (Fixup.getTargetKind() == BPF::FK_BPF_PCRel_4) { + Value = (uint32_t)((Value - 8) / 8); + support::endian::write<uint32_t>(&Data[Fixup.getOffset() + 4], Value, ---------------- This is because `Value` is in bytes, right? Could you please drop a comment here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144829/new/ https://reviews.llvm.org/D144829 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits