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

[Alex Bradbury via Phabricator via llvm-commits]

asb accepted this revision.
asb added a comment.
This revision is now accepted and ready to land.

This looks good to me, thanks! Just a few minor comments inline.



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1110
   StringRef Identifier;
+  auto tok = getLexer().getTok();
+
----------------
Should be `Tok` and I don't think this is a good case for using `auto` (see https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable). `AsmToken Tok = getLexer().getTok();` would IMHO be more readable and in line with the style used elsewhere in LLVM.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1118
+  if (Sym->isVariable()) {
+    auto V = Sym->getVariableValue(/*SetUsed*/ false);
+    if (!isa<MCSymbolRefExpr>(V)) {
----------------
`const MCExpr *V = `


================
Comment at: lib/Target/RISCV/RISCVInstrFormats.td:111
 
+// Pseudo load instructions.
+class PseudoLoad<string opcodestr, RegisterClass rdty = GPR>
----------------
It would be good to add a comment here to indicate that these are used for load/store to a symbol address.


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

https://reviews.llvm.org/D50496