[llvm-mc] Add -M to replace -riscv-no-aliases and -riscv-arch-reg-names

[MaskRay]

Summary

In objdump, many targets support -M no-aliases. Instead of having a
-*-no-aliases for each target when LLVM adds the support, it makes more sense
to introduce objdump style -M.

-riscv-arch-reg-names is removed. -riscv-no-aliases has too many uses and thus is retained for now.

[MaskRay]

add a TODO

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp

44

Will do s/can /can be / in the next diff.

[tmatheson]

If we are just keeping -riscv-no-aliases to avoid changing many tests it might be better to just remove it now. Otherwise it will likely stay forever. Maybe it is used externally though, I don't know.

llvm/include/llvm/MC/MCInstPrinter.h

58

It would be better if we could avoid ending up with two variables that mean essentially the same thing. I prefer the name NoAliases because it matches both command line flags better, and doesn't have the logical inversion that PrintAliases does. Can they both be made to use the same storage with cl::location?

[jrtc27]

I think a mass replacement of -(-)riscv-no-aliases is fine. I don't know why so many RVV CodeGen tests use it to be honest, it seems entirely unnecessary. @craig.topper / @frasercrmck is there a reason why the RVV CodeGen tests use --riscv-no-aliases everywhere? As far as I can tell there's nothing interesting hidden by aliases if I remove the option from all the RVV tests and regenerate them:

waltham:llvm-project-upstream jrtc4% git diff | grep '^[-+]; CHECK' | sort | uniq -c
  24844 -; CHECK-NEXT:    jalr zero, 0(ra)
  24844 +; CHECK-NEXT:    ret
      8 -; CHECK-NEXT:    vl4re8.v v28, (a0)
      8 +; CHECK-NEXT:    vl4r.v v28, (a0)
     64 -; CHECK-NEXT:    vl8re8.v v24, (a0)
     64 +; CHECK-NEXT:    vl8r.v v24, (a0)

So, can we just delete them all?

[frasercrmck]

I've never understood why they're there either so I'm not best-qualified to say. I've never used it when adding my own tests. As @tmatheson said, maybe it's used externally?

[craig.topper]

In D103004#2779572, @frasercrmck wrote:

I've never understood why they're there either so I'm not best-qualified to say. I've never used it when adding my own tests. As @tmatheson said, maybe it's used externally?

I'm also not sure why the rvv tests use no-aliases. I think the majority were generated by a tool developed before I get here. I'll ask around internally.

[MaskRay]

llvm/include/llvm/MC/MCInstPrinter.h

58

No. RISCV is at odds here. I will use this for aarch64 as well: D103005.

The cl::opt global variable is inelegant and should be avoided. Say we want to have MCInstPrinter, now due to the singleton nature of a cl::opt variable, we cannot really make their PrintAliases states different.

Regarding the name, The positively named PrintAliases avoid negation, instead of the other way around.
If we add something -M aliases then it should become clearer NoAliases is not a good name.

[jrtc27]

In D103004#2780185, @craig.topper wrote:

In D103004#2779572, @frasercrmck wrote:

I've never understood why they're there either so I'm not best-qualified to say. I've never used it when adding my own tests. As @tmatheson said, maybe it's used externally?

I'm also not sure why the rvv tests use no-aliases. I think the majority were generated by a tool developed before I get here. I'll ask around internally.

If you conclude they can go, I have the patch ready to push. I could put it in Phab, but I doubt anyone wants to read the 25309 changed lines; the full summary of all the changes is very boring:

waltham:llvm-project-upstream jrtc4% git diff | grep '^[-+][^-+]' | sort | uniq -c
  24844 -; CHECK-NEXT:    jalr zero, 0(ra)
  24844 +; CHECK-NEXT:    ret
      8 -; CHECK-NEXT:    vl4re8.v v28, (a0)
      8 +; CHECK-NEXT:    vl4r.v v28, (a0)
     64 -; CHECK-NEXT:    vl8re8.v v24, (a0)
     64 +; CHECK-NEXT:    vl8r.v v24, (a0)
    392 -; RUN:   --riscv-no-aliases < %s | FileCheck %s
    392 +; RUN:   < %s | FileCheck %s
      1 -; RUN:   -verify-machineinstrs --riscv-no-aliases < %s \
      1 +; RUN:   -verify-machineinstrs < %s \

[evandro]

In D103004#2779570, @jrtc27 wrote:

So, can we just delete them all?

I think so in the RVV tests, but not everywhere. Not aliasing may have a place in assembler tests, for instance, but not only in these.

[jrtc27]

In D103004#2780282, @evandro wrote:

In D103004#2779570, @jrtc27 wrote:

So, can we just delete them all?

I think so in the RVV tests, but not everywhere. Not aliasing may have a place in assembler tests, for instance, but not only in these.

Yes, I'm only talking about llvm/test/CodeGen/RISCV/rvv/*.ll (and llvm/test/CodeGen/RISCV/spill-fpr-scalar.ll which should probably be under rvv/). All the MC tests, and the special llvm/test/CodeGen/RISCV/patchable-function-entry.ll test, are excluded.

[evandro]

In D103004#2780185, @craig.topper wrote:

I'm also not sure why the rvv tests use no-aliases. I think the majority were generated by a tool developed before I get here. I'll ask around internally.

I believe that they were lifted from existing tests by default. When looking for specific instructions, having the actual instruction makes it clearer what the test expects. But this option is probably redundant in tests that don't check for instructions.

[MaskRay]

Does this patch look good?

[luismarques]

I'm not very happy that we now have to consistently check two variables (error-prone, etc.), but I suppose that's OK if we are going to remove -riscv-no-aliases soon.
Other than that, the patch LGTM.

[craig.topper]

In D103004#2780266, @jrtc27 wrote:

In D103004#2780185, @craig.topper wrote:

In D103004#2779572, @frasercrmck wrote:

I've never understood why they're there either so I'm not best-qualified to say. I've never used it when adding my own tests. As @tmatheson said, maybe it's used externally?

I'm also not sure why the rvv tests use no-aliases. I think the majority were generated by a tool developed before I get here. I'll ask around internally.

If you conclude they can go, I have the patch ready to push. I could put it in Phab, but I doubt anyone wants to read the 25309 changed lines; the full summary of all the changes is very boring:

waltham:llvm-project-upstream jrtc4% git diff | grep '^[-+][^-+]' | sort | uniq -c
  24844 -; CHECK-NEXT:    jalr zero, 0(ra)
  24844 +; CHECK-NEXT:    ret
      8 -; CHECK-NEXT:    vl4re8.v v28, (a0)
      8 +; CHECK-NEXT:    vl4r.v v28, (a0)
     64 -; CHECK-NEXT:    vl8re8.v v24, (a0)
     64 +; CHECK-NEXT:    vl8r.v v24, (a0)
    392 -; RUN:   --riscv-no-aliases < %s | FileCheck %s
    392 +; RUN:   < %s | FileCheck %s
      1 -; RUN:   -verify-machineinstrs --riscv-no-aliases < %s \
      1 +; RUN:   -verify-machineinstrs < %s \

They can go.

[jrtc27]

In D103004#2782499, @craig.topper wrote:

In D103004#2780266, @jrtc27 wrote:

In D103004#2780185, @craig.topper wrote:

In D103004#2779572, @frasercrmck wrote:

I've never understood why they're there either so I'm not best-qualified to say. I've never used it when adding my own tests. As @tmatheson said, maybe it's used externally?

I'm also not sure why the rvv tests use no-aliases. I think the majority were generated by a tool developed before I get here. I'll ask around internally.

If you conclude they can go, I have the patch ready to push. I could put it in Phab, but I doubt anyone wants to read the 25309 changed lines; the full summary of all the changes is very boring:

waltham:llvm-project-upstream jrtc4% git diff | grep '^[-+][^-+]' | sort | uniq -c
  24844 -; CHECK-NEXT:    jalr zero, 0(ra)
  24844 +; CHECK-NEXT:    ret
      8 -; CHECK-NEXT:    vl4re8.v v28, (a0)
      8 +; CHECK-NEXT:    vl4r.v v28, (a0)
     64 -; CHECK-NEXT:    vl8re8.v v24, (a0)
     64 +; CHECK-NEXT:    vl8r.v v24, (a0)
    392 -; RUN:   --riscv-no-aliases < %s | FileCheck %s
    392 +; RUN:   < %s | FileCheck %s
      1 -; RUN:   -verify-machineinstrs --riscv-no-aliases < %s \
      1 +; RUN:   -verify-machineinstrs < %s \

They can go.

Done :) https://reviews.llvm.org/rGd63d662d3cc51219fb08908ebea8d5851e53adb8

llvm/include/llvm/MC/MCInstPrinter.h

protected:
   const MCAsmInfo &MAI;
   const MCInstrInfo &MII;
   const MCRegisterInfo &MRI;
   const MCInstrAnalysis *MIA = nullptr;
 
   /// True if we are printing marked up assembly.
   bool UseMarkup = false;
 
+  /// True if we prefer aliases (e.g. nop) to raw mnemonics.
+  bool PrintAliases = true;
+
   /// True if we are printing immediates as hex.
   bool PrintImmHex = false;
 
   /// Which style to use for printing hexadecimal values.
   HexStyle::Style PrintHexStyle = HexStyle::C;
 
   /// If true, a branch immediate (e.g. bl 4) will be printed as a hexadecimal
   /// address (e.g. bl 0x20004). This is useful for a stream disassembler

llvm/lib/Target/RISCV/MCTargetDesc/RISCVInstPrinter.cpp

 #define GEN_UNCOMPRESS_INSTR
 #include "RISCVGenCompressInstEmitter.inc"
 
 static cl::opt<bool>
     NoAliases("riscv-no-aliases",
               cl::desc("Disable the emission of assembler pseudo instructions"),
               cl::init(false), cl::Hidden);
 
-static cl::opt<bool>
-    ArchRegNames("riscv-arch-reg-names",
-                 cl::desc("Print architectural register names rather than the "
-                          "ABI names (such as x2 instead of sp)"),
-                 cl::init(false), cl::Hidden);
+// Print architectural register names rather than the ABI names (such as x2
+// instead of sp).
+// TODO: Make RISCVInstPrinter::getRegisterName non-static so that this can a
+// member.
+static bool ArchRegNames;
 
 // The command-line flags above are used by llvm-mc and llc. They can be used by
 // `llvm-objdump`, but we override their values here to handle options passed to
 // `llvm-objdump` with `-M` (which matches GNU objdump). There did not seem to
 // be an easier way to allow these options in all these tools, without doing it
 // this way.
 bool RISCVInstPrinter::applyTargetSpecificCLOption(StringRef Opt) {
   if (Opt == "no-aliases") {
-    NoAliases = true;
+    PrintAliases = false;
     return true;
   }
   if (Opt == "numeric") {
     ArchRegNames = true;
     return true;
   }
 
   return false;
 }
 
 void RISCVInstPrinter::printInst(const MCInst *MI, uint64_t Address,
                                  StringRef Annot, const MCSubtargetInfo &STI,
                                  raw_ostream &O) {
   bool Res = false;
   const MCInst *NewMI = MI;
   MCInst UncompressedMI;
-  if (!NoAliases)
+  if (PrintAliases && !NoAliases)
     Res = uncompressInst(UncompressedMI, *MI, MRI, STI);
   if (Res)
     NewMI = const_cast<MCInst *>(&UncompressedMI);
-  if (NoAliases || !printAliasInstr(NewMI, Address, STI, O))
+  if (!PrintAliases || NoAliases || !printAliasInstr(NewMI, Address, STI, O))
     printInstruction(NewMI, Address, STI, O);
   printAnnotation(O, Annot);
 }
 
 void RISCVInstPrinter::printRegName(raw_ostream &O, unsigned RegNo) const {
   O << getRegisterName(RegNo);
 }
 

llvm/test/MC/RISCV/numeric-reg-names-d.s

-# RUN: llvm-mc -triple riscv32 -mattr=+f,+d < %s -riscv-arch-reg-names \
+# RUN: llvm-mc -triple riscv32 -mattr=+f,+d -M numeric < %s \
 # RUN:     | FileCheck -check-prefix=CHECK-NUMERIC %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+f,+d < %s \
 # RUN:     | llvm-objdump --mattr=+f,+d -d -M numeric - \
 # RUN:     | FileCheck -check-prefix=CHECK-NUMERIC %s
 
 # CHECK-NUMERIC: fsqrt.d f10, f0
 # CHECK-NUMERIC-NEXT: fsqrt.d f10, f0
 fsqrt.d fa0, f0

llvm/test/MC/RISCV/numeric-reg-names-f.s

-# RUN: llvm-mc -triple riscv32 -mattr=+f < %s -riscv-arch-reg-names \
+# RUN: llvm-mc -triple riscv32 -mattr=+f -M numeric < %s \
 # RUN:     | FileCheck -check-prefix=CHECK-NUMERIC %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 -mattr=+f < %s \
 # RUN:     | llvm-objdump --mattr=+f -d -M numeric - \
 # RUN:     | FileCheck -check-prefix=CHECK-NUMERIC %s
 
 # CHECK-NUMERIC: fsqrt.s f10, f0
 # CHECK-NUMERIC-NEXT: fsqrt.s f10, f0
 fsqrt.s fa0, f0

llvm/test/MC/RISCV/numeric-reg-names.s

-# RUN: llvm-mc -triple riscv32 < %s -riscv-arch-reg-names \
+# RUN: llvm-mc -triple riscv32 -M numeric %s \
 # RUN:     | FileCheck -check-prefix=CHECK-NUMERIC %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
 # RUN:     | llvm-objdump -d -M numeric - \
 # RUN:     | FileCheck -check-prefix=CHECK-NUMERIC %s
 
 # CHECK-NUMERIC: addi x10, x0, 1
 # CHECK-NUMERIC-NEXT: addi x10, x0, 1
 addi a0, x0, 1

llvm/test/MC/RISCV/rvi-aliases-valid.s

-# RUN: llvm-mc %s -triple=riscv32 -riscv-no-aliases \
+# RUN: llvm-mc %s -triple=riscv32 -M no-aliases \
 # RUN:     | FileCheck -check-prefixes=CHECK-S-NOALIAS,CHECK-S-OBJ-NOALIAS %s
 # RUN: llvm-mc %s -triple=riscv32 \
 # RUN:     | FileCheck -check-prefixes=CHECK-S,CHECK-S-OBJ %s
-# RUN: llvm-mc %s -triple=riscv64 -riscv-no-aliases\
+# RUN: llvm-mc %s -triple=riscv64 -M no-aliases \
 # RUN:     | FileCheck -check-prefixes=CHECK-S-NOALIAS,CHECK-S-OBJ-NOALIAS %s
 # RUN: llvm-mc %s -triple=riscv64 \
 # RUN:     | FileCheck -check-prefixes=CHECK-S,CHECK-S-OBJ %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
 # RUN:     | llvm-objdump -d -r -M no-aliases - \
 # RUN:     | FileCheck -check-prefixes=CHECK-OBJ-NOALIAS,CHECK-S-OBJ-NOALIAS %s
 # RUN: llvm-mc -filetype=obj -triple riscv32 < %s \
 # RUN:     | llvm-objdump -d -r - \

llvm/test/tools/llvm-mc/disassembler-options.test

+# RUN: not llvm-mc -M invalid /dev/null 2>&1 | FileCheck %s
+
+# CHECK: error: invalid disassembler option 'invalid'

llvm/tools/llvm-mc/llvm-mc.cpp

 
 using namespace llvm;
 
 static mc::RegisterMCTargetOptionsFlags MOF;
 
 static cl::opt<std::string>
 InputFilename(cl::Positional, cl::desc("<input file>"), cl::init("-"));
 
+static cl::list<std::string>
+    DisassemblerOptions("M", cl::desc("Disassembler options"));
+
 static cl::opt<std::string> OutputFilename("o", cl::desc("Output filename"),
                                            cl::value_desc("filename"),
                                            cl::init("-"));
 
 static cl::opt<std::string> SplitDwarfFile("split-dwarf-file",
                                            cl::desc("DWO output filename"),
                                            cl::value_desc("filename"));
 
if (FileType == OFT_AssemblyFile) {
     if (!IP) {
       WithColor::error()
           << "unable to create instruction printer for target triple '"
           << TheTriple.normalize() << "' with assembly variant "
           << OutputAsmVariant << ".\n";
       return 1;
     }
 
+    for (StringRef Opt : DisassemblerOptions)
+      if (!IP->applyTargetSpecificCLOption(Opt)) {
+        WithColor::error() << "invalid disassembler option '" << Opt << "'\n";
+        return 1;
+      }
+
     // Set the display preference for hex vs. decimal immediates.
     IP->setPrintImmHex(PrintImmHex);
 
     // Set up the AsmStreamer.
     std::unique_ptr<MCCodeEmitter> CE;
     if (ShowEncoding)
       CE.reset(TheTarget->createMCCodeEmitter(*MCII, *MRI, Ctx));