[PATCH] D50496: [RISCV] Implment pseudo instructions for load/store from a symbol address.

[James Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1230
+///   TmpLabel: AUIPC rdest, %pcrel_hi(symbol)
+const MCExpr *RISCVAsmParser::emitLoadSymbolAddress(const MCExpr *Symbol,
+                                                    const MCOperand &TmpReg,
----------------
kito-cheng wrote:
> rogfer01 wrote:
> > Suggestion: what about renaming it `emitLoadSymbolAddressHi`, it might look like the full symbol address will be computed otherwise. Then you can rename the new `emitLoadWithSymbol` to just `emitLoadSymbol`.
> Thanks your suggestion, naming is always hard to me :P
It strikes me that, apart from choosing the opcode (add/load/store) and store having a temporary register, `emitLoadLocalAddress`/`emitLoadSymbol`/`emitStoreSymbol` all do exactly the same thing. Can we not move all that into `emitLoadSymbolAddressHi`, rename it appropriately (something like `emitAuipcInstPair` as an idea) and then remove `emitLoadLocalAddress`/`emitLoadSymbol`/`emitStoreSymbol`? Then `processInstruction` would look something like:

```
  // ...
  case RISCV::PseudoLLA:
    emitAuipcInstPair(Inst, RISCV::ADDI, IDLoc, Out);
    return false;
  case RISCV::PseudoLB:
    emitAuipcInstPair(Inst, RISCV::LB, IDLoc, Out);
    return false;
  // ...
  case RISCV::PseudoSB:
    emitAuipcInstPair(Inst, RISCV::SB, IDLoc, Out);
    return false;
  // ...
```

Note that `Inst.getNumOperands()` tells you whether there is a temporary register to use.

Alternatively, you can make `emitAuipcInstPair` take `unsigned Opcode, MCOperand DestReg, MCOperand TmpReg, MCExpr *Symbol, SMLoc IDLoc, MCStreamer &Out` and keep your wrappers, but their only job is to unpack `DestReg`, `Symbol` and `TmpReg` (set to `DestReg` for add/load). I'd still push the opcode selection out into `processInstruction` as you already have to enumerate the pseudo opcodes there. Then you'd have something like:

```
void RISCVAsmParser::emitLoadLocalAddress(MCInst &Inst, SMLoc IDLoc,
                                          MCStreamer &Out) {
  // The local load address pseudo-instruction "lla" is used in PC-relative
  // addressing of symbols:
  //   lla rdest, symbol
  // expands to
  //   TmpLabel: AUIPC rdest, %pcrel_hi(symbol)
  //             ADDI rdest, %pcrel_lo(TmpLabel)
  MCOperand DestReg = Inst.getOperand(0);
  const MCExpr *Symbol = Inst.getOperand(1).getExpr();
  emitAuipcInstPair(RISCV::ADDI, DestReg, DestReg, Symbol, IDLoc, Out);
}

void RISCVAsmParser::emitLoadSymbol(MCInst &Inst, unsigned Opcode, SMLoc IDLoc,
                                    MCStreamer &Out) {
  // The load pseudo-instruction does a pc-relative load with
  // a symbol.
  //
  // The expansion looks like this
  //
  //   TmpLabel: AUIPC rd, %pcrel_hi(symbol)
  //             LX    rd, %pcrel_lo(TmpLabel)(rd)
  MCOperand DestReg = Inst.getOperand(0);
  const MCExpr *Symbol = Inst.getOperand(1).getExpr();
  emitAuipcInstPair(Opcode, DestReg, DestReg, Symbol, IDLoc, Out);
}

void RISCVAsmParser::emitStoreSymbol(MCInst &Inst, unsigned Opcode, SMLoc IDLoc,
                                    MCStreamer &Out) {
  // The store pseudo-instruction does a pc-relative store with
  // a symbol.
  //
  // The expansion looks like this
  //
  //   TmpLabel: AUIPC tmp, %pcrel_hi(symbol)
  //             SX    rd, %pcrel_lo(TmpLabel)(tmp)
  MCOperand DestReg = Inst.getOperand(0);
  MCOperand TmpReg = Inst.getOperand(1);
  const MCExpr *Symbol = Inst.getOperand(2).getExpr();
  emitAuipcInstPair(Opcode, DestReg, TmpReg, Symbol, IDLoc, Out);
}
```

Although we can be smart and distinguish between stores and non-stores as described above, I'm actually leaning towards this one as it's a bit more explicit, clearer, and will catch cases where we do something stupid (e.g. stores without a temporary register).


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

https://reviews.llvm.org/D50496