[PATCH] D40902: [RISCV] implemented assembler pseudo instructions for RV32I and RV64I

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Just a few minor niggles in the inline comments. Thanks for matching the table in the ISA manual so directly - it made this trivial to review.

See my inline comments about enabling this by default - I'm pretty open to doing this however you prefer, so it's up to you.

Are you planning to submit a follow-up patch for the floating point pseudoinstructions?



================
Comment at: lib/Target/RISCV/InstPrinter/RISCVInstPrinter.cpp:33
+static cl::opt<bool>
+AliasInstrEmission("riscv-alias-instr-emission",
+            cl::desc("Enable the emission of assembler pseudo instructions"),
----------------
Perhaps a name that better aligns with the -no-aliases flag used by the GNU tools would make sense? `-riscv-no-aliases` and `NoAliases`. We should probably emit aliases by default, matching the GNU behaviour.  Of course that's a more disruptive changes, so as an alternative:
* Update this patch to use `riscv-aliases`, disabled by default.
* At a later point, I can switch it to a `riscv-no-aliases` argument that is false by default and update the test cases as necessary.

Only issue with the incremental approach is that it interferes with the current use of InstAlias for the rounding mode in floating point instructions. If it doesn't break the floating point MC layer tests (due to not matching to the end of the line), it probably should.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:504
+let Predicates = [IsRV64] in {
+  def : InstAlias<"negw $rd, $rs",   (SUBW  GPR:$rd,      X0, GPR:$rs)>;
+  def : InstAlias<"sext.w $rd, $rs", (ADDIW GPR:$rd, GPR:$rs,       0)>;
----------------
For what it's worth I tend to indent these as if they were C++ namespaces (which LLVM coding style [prefers not to indent](http://llvm.org/docs/CodingStandards.html#namespace-indentation)). You'd probably want to add an extra newline before/after to separate these RV64-only aliases visually though.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:525
+
+// always output the canonical mnemonic for the pseudo branch instructions
+def : InstAlias<"bgt $rs, $rt, $offset",
----------------
Apologies that this seems incredibly picky, but LLVM style prefers [full sentences as comments](http://llvm.org/docs/CodingStandards.html#commenting) - i.e. capitalised first letter, end with a full stop.

Thanks for spotting this behaviour. I'd recommend documenting that we do this to match GNU behaviour, e.g. "The GNU tools always emit the canonical mnemonic for the branch pseudoinstructions (e.g. bgt will be recognised by the assembler but never printed by objdump). Match this behaviour by setting a zero weight."


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:548
+// 0xC80 == cycleh, 0xC81 == timeh, 0xC82 == instreth
+def : InstAlias<"rdcycle $rd",   (CSRRS GPR:$rd, 0xC00, X0)>;
+def : InstAlias<"rdtime $rd",    (CSRRS GPR:$rd, 0xC01, X0)>;
----------------
Given that you've maintained the same ordering as the tables in the ISA manual everywhere else (thanks! That's made is really easy to review), I think it would make sense to order these as rdinstret, rdcyle, rdtime to match table 20.3 in the spec. Same for the RV32-only rd*h variants.


https://reviews.llvm.org/D40902