Page MenuHomePhabricator

[Aarch64AsmPrinter] support %c output template for global addresses
AbandonedPublic

Authored by nickdesaulniers on Apr 8 2019, 4:44 PM.

Details

Summary

The Linux kernel makes use of %c<digit> "output template" in extended
inline assembly, particularly with asm goto for static key runtime
patching. The inline assembly uses the address of a global variable as
input to the extended inline assembly.

There's generic handling of %c in AsmPrinter::PrintAsmOperand() for
immediates. I considered extending support for global addresses there,
but it seems that X86AsmPrinter::PrintAsmOperand() has quite a bit of
custom logic for printing global addresses (see printSymbolOperand() in
X86AsmPrinter.cpp).

Fixes: https://bugs.llvm.org/show_bug.cgi?id=41402
Link: https://gcc.gnu.org/onlinedocs/gccint/Output-Template.html#Output-Template

Event Timeline

nickdesaulniers created this revision.Apr 8 2019, 4:44 PM

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, but I'm happy to let that pass if others prefer it this way.

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
499

I think this comment will need updating to mention that the implementation of 'c' is split between AsmPrinter::PrintAsmOperand and this function,

509

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.

nickdesaulniers marked 3 inline comments as done.Apr 9 2019, 10:03 AM

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?

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp
509

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.

nickdesaulniers marked an inline comment as done.Apr 9 2019, 11:01 AM

How does that sound?

eh, looks like the arch specific asm printers have printOperand as well that would need to be hoisted to the base class in order to properly handle %c in a generic way. That will likely be a bigger refactor to AsmPrinter and derived classes. I think I'm going to do that in a separate commit first, then revisit this one.

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?

That sounds reasonable to me. I think some backends such as Power and Hexagon have explicit handling for 'c' that does nothing:

case 'c': // Don't print "$" before a global var name or constant.
  break; // PPC never has a prefix.

Will be worth checking all the backends for custom handling.