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

[Kito Cheng via Phabricator via llvm-commits]

kito-cheng marked 2 inline comments as done.
kito-cheng added inline comments.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:995
     return Name == "tail" || Name == "call";
   case 1:
     // lla rdest, imm
----------------
rogfer01 wrote:
> You will need to extend this case otherwise symbols named like registers fail to parse. I ran into this same problem! :)
> 
> That said, maybe we want to rethink this because your change adds a bunch of pseudos to this case.
> 
> ```
> t.s:1:8: error: immediate must be an integer in the range [-2048, 2047]
> lb a0, zero
>        ^
> t.s:2:8: error: immediate must be an integer in the range [-2048, 2047]
> sb a3, zero, a1
>        ^
> ```
Thanks, I was worried it will not work for those pseudo load/store, because the have another non-pseudo version, but it seems work fine :)


================
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,
----------------
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


https://reviews.llvm.org/D50496