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

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Thanks for the update. The helper function for force immediate is a good idea - just a couple of minor comments there.

It would be nice if there was a sensible way to merge the tests in to rvi-aliases-valid.s and a new rvi-aliases-invalid.s, but I see that CHECK-EXPAND (as used by li) isn't appropriate for lla, because of course we won't see %pcrel_hi/%pcrel_lo in the objdump output.

With the suggested tweaks to the helper function I think this is good to go. Thanks!



================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:969
 }
 
+static bool ForceImmediateOperand(StringRef Name, unsigned OperandIdx) {
----------------
I think we'd benefit from a comment here describing the function. How about:

Return true if the operand at the OperandIdx for opcode Name should be 'forced' to be parsed as an immediate. This is required for pseudoinstructions such as tail or call, which allow bare symbols to be used that could clash with register names.


================
Comment at: lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:970
 
+static bool ForceImmediateOperand(StringRef Name, unsigned OperandIdx) {
+  // FIXME: This may not scale so perhaps we want to use a data-driven approach
----------------
Function names should start with a lower-case letter. ParseInstruction etc don't do this as they override an existing method, which doesn't conform with this naming convention. https://llvm.org/docs/CodingStandards.html#id42. Maybe `shouldForceImediateOperand` would be better given the bool return type?


https://reviews.llvm.org/D49661