[PATCH] D45859: [RISCV] Support "call" pseudoinstruction in the MC layer

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks for the updates Shiva. I've added a few more suggested changes, and I think we're almost there.

I think the patch description also needs to be updated to justify why the CALL->AUIPC+JALR instruction should be as late as RISCVMCCodeEmitter and can't be earlier (e.g. RISCVAsmPrinter).



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:174-186
+  bool isCallTarget() const {
+    int64_t Imm;
+    RISCVMCExpr::VariantKind VK;
+    if (!isImm())
+      return false;
+    bool IsConstantImm = evaluateConstantImm(Imm, VK);
+    bool IsValid;
----------------
When possible, the backend tries to use generic names for operand types and predicates. e.g. isSimm21. Lets make this `isBareSymbol`. this can be implemented as below:


```
  bool isBareSymbol() const {
    int64_t Imm;
    RISCVMCExpr::VariantKind VK;
    // Must be of 'immediate' type but not a constant.
    if (!isImm() || evaluateConstantImm(Imm, VK))
      return false;
    return RISCVAsmParser::classifySymbolRef(getImm(), VK, Imm) &&
           VK == RISCVMCExpr::VK_RISCV_None;
  }
```


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:495-499
+  void addCallTargetOperands(MCInst &Inst, unsigned N) const {
+    assert(N == 1 && "Invalid number of operands!");
+    addExpr(Inst, getImm());
+  }
+
----------------
Duplicates addImmOperands. Delete.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:725-730
+  case Match_InvalidCallTarget: {
+    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
+    return Error(
+        ErrorLoc,
+        "operand must be symbol of function name");
+  }
----------------
```
  case Match_InvalidBareSymbol: {
    SMLoc ErrorLoc = ((RISCVOperand &)*Operands[ErrorInfo]).getStartLoc();
    return Error(ErrorLoc, "operand must be a bare symbol name");
  }

```


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:89
+// Expand PseudoCALL to AUIPC and JALR with relocation types.
+// We expand PseudoCALL while encoding, So AUIPC and JALR won't go through
+// RISCV MC to MC compressed instruction transformation. It's ok because
----------------
So -> meaning


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:90
+// We expand PseudoCALL while encoding, So AUIPC and JALR won't go through
+// RISCV MC to MC compressed instruction transformation. It's ok because
+// AUIPC has no 16-bit form and C_JALR have no immediate operand field.
----------------
"It's ok because" -> "This is acceptable because".


================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp:103
+
+  assert(Func.isExpr());
+
----------------
 `assert(Func.isExpr() && "Expected expression");`


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:156-169
+def CallTarget : AsmOperandClass {
+  let Name = "CallTarget";
+  let RenderMethod = "addCallTargetOperands";
+  let DiagnosticType = "InvalidCallTarget";
+}
+
+// A function call target symbol.
----------------
```
def BareSymbol : AsmOperandClass {
  let Name = "BareSymbol";
  let RenderMethod = "addImmOperands";
  let DiagnosticType = "InvalidBareSymbol";
}

// A bare symbol.
def bare_symbol : Operand<XLenVT> {
  let ParserMatchClass = BareSymbol;
  let MCOperandPredicate = [{
     return MCOp.isBareSymbolRef();
  }];
}

```


Repository:
  rL LLVM

https://reviews.llvm.org/D45859