[PATCH] D157806: [BPF] Fix in/out argument constraints for CORE_MEM instructions

[Eduard Zingerman via Phabricator via llvm-commits]

eddyz87 added a comment.

In D157806#4583421 <https://reviews.llvm.org/D157806#4583421>, @yonghong-song wrote:

> ... Another solution is similar to D157805 <https://reviews.llvm.org/D157805> where we can clear 'killed' for the 'out' parameter, right?

The error is reported by the following code in the `MachineVerifier.cpp`:

  void
  MachineVerifier::visitMachineOperand(const MachineOperand *MO, unsigned MONum) {
    const MachineInstr *MI = MO->getParent();
    const MCInstrDesc &MCID = MI->getDesc();
    unsigned NumDefs = MCID.getNumDefs();
    if (MCID.getOpcode() == TargetOpcode::PATCHPOINT)
      NumDefs = (MONum == 0 && MO->isReg()) ? NumDefs : 0;
  
    // The first MCID.NumDefs operands must be explicit register defines
    if (MONum < NumDefs) {
      const MCOperandInfo &MCOI = MCID.operands()[MONum];
      if (!MO->isReg())
        report("Explicit definition must be a register", MO, MONum);
      else if (!MO->isDef() && !MCOI.isOptionalDef())
        report("Explicit definition marked as use", MO, MONum);  <---- error is reported here
      else if (MO->isImplicit())
        report("Explicit definition marked as implicit", MO, MONum);
    } else if (...) {
      ...
    }
    ...
  }

As far as I understand this code:

- `MCID` is a static information about instruction, generated from the `td` file;
- `MCID.getNumDefs()` is the number of values specified in the `outs` part of the pattern (e.g. for `CORE_MEM` outs defined as `(outs GPR:$dst)`, hence `getNumDefs()` returns `1`);
- to satisfy the above check on the machine operand level the following change is needed:

  @@ -146,8 +147,11 @@ void BPFMISimplifyPatchable::checkADDrr(MachineRegisterInfo *MRI,
           continue;
       }
   
  +    MachineOperand &FirstMO = DefInst->getOperand(0);
  +    FirstMO.setIsKill(false); // Can't setIsDef(true) if kill is set
  +    FirstMO.setIsDef(true);
       BuildMI(*DefInst->getParent(), *DefInst, DefInst->getDebugLoc(), TII->get(COREOp))
  -        .add(DefInst->getOperand(0)).addImm(Opcode).add(*BaseOp)
  +        .add(FirstMO).addImm(Opcode).add(*BaseOp)
           .addGlobalAddress(GVal);
       DefInst->eraseFromParent();
     }

But this is not conservative for stores (as store does not define it's first operand) and also leads to incorrect SSA form, which is reported by the MachineVerifier:

  bb.0.entry:
    liveins: $r1
    DBG_VALUE $r1, $noreg, !"t", !DIExpression(), debug-location !18; t.c:0 line no:5
    %0:gpr = COPY $r1
    DBG_VALUE %0:gpr, $noreg, !"t", !DIExpression(), debug-location !18; t.c:0 line no:5
    %1:gpr = LD_imm64 @"llvm.t:0:0$0:0"
    %3:gpr = ADD_rr %0:gpr(tied-def 0), killed %1:gpr
    %4:gpr = MOV_ri 42
    %4:gpr = CORE_MEM 411, %0:gpr, @"llvm.t:0:0$0:0", debug-location !19; t.c:6:8
    RET debug-location !25; t.c:7:1
  
  # End machine code for function test.
  
  *** Bad machine code: Multiple virtual register defs in SSA form ***
  - function:    test
  - basic block: %bb.0 entry (0x621000068290)
  - instruction: %4:gpr = MOV_ri 42
  - operand 0:   %4:gpr
  
  *** Bad machine code: Multiple virtual register defs in SSA form ***
  - function:    test
  - basic block: %bb.0 entry (0x621000068290)
  - instruction: %4:gpr = CORE_MEM 411, %0:gpr, @"llvm.t:0:0$0:0", debug-location !19; t.c:6:8
  - operand 0:   %4:gpr

Which complains about two definitions for `%4`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157806/new/

https://reviews.llvm.org/D157806