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

[Mario Werner via Phabricator via llvm-commits]

niosHD added a comment.

Thank you for the review Alex. I will incorporate your comments in an updated patch today.

I definitely plan to submit a follow-up patch with the remaining floating point alias instructions once this one is finished.



================
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"),
----------------
asb wrote:
> 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.
Matching the GNU tools flag name definitely makes sense since it makes it easier for users to find it. I will therefore switch to your proposed `-riscv-no-aliases` flag with the `NoAliases` variable name.

Regarding default behaviour I also planed to match the GNU tools eventually. However, regarding the process I am not sure myself and would appreciate your opinion. If you don't think it adds too much noise to the patch to update all affected test cases then I can directly integrate it. Otherwise, I thought about flipping the default and updating the test cases in a separate patch.

Interestingly, I don't get a test failure in the floating point MC tests due to disabling  the printing of the aliases by default with this patch. I need to have a closer look why this is the case.


================
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)>;
----------------
asb wrote:
> 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.
Sure, will be updated.


================
Comment at: lib/Target/RISCV/RISCVInstrInfo.td:525
+
+// always output the canonical mnemonic for the pseudo branch instructions
+def : InstAlias<"bgt $rs, $rt, $offset",
----------------
asb wrote:
> 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."
No worries, since it is my first upstream patch I am grateful for your detailed comments. I simply was not aware of that convention and will update it.

Regarding behaviour you give me too much credit. I simply thought that emitting different branching instructions is not worth the complexity. However, I will add a sentence which documents that the GNU tools do the same.


================
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)>;
----------------
asb wrote:
> 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.
Sure, I will update the order. Thank you for noticing.


https://reviews.llvm.org/D40902