[PATCH] D155896: [Target][MC] Cleaning up AssemblerDialect / InstructionPrinterSyntaxVariant

[Christoph Stiller via Phabricator via llvm-commits]

rainerzufalldererste added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp:120-137
+  // Resolve `InlineAsm::AsmDialect` to `AsmDialect`.
+  AsmDialect::Type CorrespondingAsmDialect;
+
+  switch (Dialect) {
+  case InlineAsm::AD_ATT:
+    CorrespondingAsmDialect = AsmDialect::X86_ATT;
+    break;
----------------
This is where I expected to be a good place for a point of separation - but my initial version also transitioned all of InlineAsm to the new enum.
I'm not sure how the inline assembly is supported across different target platforms, so please let me know whether that separation is justified.


================
Comment at: llvm/lib/MC/MCDisassembler/Disassembler.cpp:324-325
+      AsmDialect::Type AsmPrinterVariant = MAI->getAssemblerDialect();
+      // what is the following line supposed to achieve? Do we want this?
+      AsmPrinterVariant = (AsmDialect::Type)(AsmPrinterVariant == 0 ? 1 : 0);
       MCInstPrinter *IP = DC->getTarget()->createMCInstPrinter(
----------------
The comment obviously needs to be removed, but I wasn't sure what the line is trying to achieve and there's probably a cleaner way to do it now.


================
Comment at: llvm/lib/Target/SystemZ/MCTargetDesc/SystemZMCAsmInfo.h:18
 class Triple;
-enum SystemZAsmDialect { AD_ATT = 0, AD_HLASM = 1 };
 
----------------
Would you prefer to keep this around, even through it's now unused (for potential backwards-compatibility)?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155896/new/

https://reviews.llvm.org/D155896