[PATCH] D144829: [WIP][BPF] Add a few new insns under cpu=v4

[Eduard Zingerman via Phabricator via cfe-commits]

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