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

Alex Bradbury via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 14 02:07:10 PST 2019

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.



More information about the llvm-commits mailing list