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

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill 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,
----------------
jrtc27 wrote:
> 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).
Can this implementation of `emitAuipcInstPair` be brought in line with the same function as implemented in D55325, or vice-versa? I would prefer @jrtc27's second suggestion here as has been implemented in D55325, simply because `emitLoadAddress` requires a wrapper anyway, and `emitLoadLocalAddress` would have to be re-added in that patch if it were to be deleted here.


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

https://reviews.llvm.org/D50496