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

[Alex Bradbury via Phabricator via llvm-commits]

asb added inline comments.


================
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"),
----------------
niosHD wrote:
> 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.
I think the reason the floating point MC tests don't break is that the FileCheck doesn't match to the end of the line, so the extra format argument is just ignored. Adding `--match-full-lines` to the FileCheck invocation won't work, but presumably a regex can be written to check for end-of-line or space and tighten the checks up.

I think the best way of managing the switch is probably:
* This patch leaves alias emission disabled by default. Note in the patch summary that it really should break the floating point MC tests but doesn't (and this will be addressed in a follow-up patch).
* A follow-up patch enables alias emissions, updates the tests, and ideally tightens up the floating point MC layer tests

That way if there are unexpected issues it's easy to revert the patch that enables aliases by default.


https://reviews.llvm.org/D40902