[PATCH] D49661: [RISCV] Add "lla" pseudo-instruction to assembler

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks for the patch, just a few minor comments in-line.

Could you add a test that shows non-bare symbols are rejected? e.g. `lla a0, %lo(sym)`?

This will fail to parse lla where the symbol coincides with a register name. Take a look at 'ForceImmediate' as used when parsing call and tail - you'll need to do something similar here. Due to the lack of clear separation between the register and symbol name namespaces for these pseudo-instructions, I think this kind of hack in the parser is the only way to go.



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1192
+                                          MCStreamer &Out) {
+  // PC-rel addressing
+  MCContext &Ctx = getContext();
----------------
Could you make the comment more descriptive? e.g. explaining the load local address pseudo-instruction.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1214-1216
+                    .addOperand(DestReg)
+                    .addOperand(DestReg)
+                    .addExpr(RefToLinkTmpLabel);
----------------
clang-format prefers slightly different indentation here


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:715-717
+def PseudoLLA : Pseudo<(outs GPR:$dst), (ins bare_symbol:$src), []> {
+  let AsmString = "lla\t$dst,$src";
+}
----------------
It would be slightly more consistent with other instruction definitions to write this as:

```
def PseudoLLA : Pseudo<(outs GPR:$dst), (ins bare_symbol:$src), [],
                       "lla", "$dst, $src">;
```


https://reviews.llvm.org/D49661