[PATCH] D60428: [Aarch64AsmPrinter] support %c output template for global addresses

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers marked 3 inline comments as done.
nickdesaulniers added a comment.

> I have a mild preference for following most of the other backends so that we don't split the handling of 'c' across the generic and the target specific

Thinking more about this last night; it's going to be a pain/stupid to keep reimplementing this for other arches.  The Linux kernel is using `%c` with global addresses for arm32 and ppc (and many others).  I think I should:

1. Create `AsmPrinter::printSymbolOperand()`, a virtual method of the generic class, that simply forwards its arguments to the virtual method `printAsmOperand()`.
2. Update `X86AsmPrinter::printSymbolOperand()` to be a member function rather than `static` local function, overriding the base `AsmPrinter::printSymbolOperand()`.
3. Handle `%c` in the generic `AsmPrinter::PrintAsmOperand()`, deferring to the now virtual method `printSymbolOperand()` for `MachineOperand::MO_GlobalAddress`, thus supporting `%c` w/ `MachineOperand::MO_GlobalAddress` for all architectures.

How does that sound?



================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:509
     default:
       return true; // Unknown modifier.
     case 'a':      // Print 'a' modifier
----------------
peter.smith wrote:
> Most other Targets, with RISCV being the exception have a return AsmPrinter::PrintAsmOperand(MI, OpNum, AsmVariant, ExtraCode, O)); here. That would mean that we could have all the implementation of 'c' in the switch statement. This does duplicate the handling of immediates here, but it is more consistent with the other targets and does mean all the logic is in one place.
Right, this tripped me up when I made the comment in the bug (https://bugs.llvm.org/show_bug.cgi?id=41402#c3).  It's called before the switch (L499) vs in the default case.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60428