Adding Comment Annotations to Outlined Functions

[AndrewLitteken]

Summary

To help explain how a function was outlined the way it was, function annotations are adde in the comments of the output assembly to explain the results. This is done by added Metadata to the function, and, when in verbose mode, when the name of the function is put in the comments near the label for the function, how it was outlined is explained next to the name.

[paquette]

Added some comments.

Also, this needs a testcase which shows that the comments are added.

I think it would make sense to get this going properly for AArch64 first, and ignore X86 for now.

llvm/include/llvm/CodeGen/MachineOutliner.h

26 ↗

(On Diff #257136)

These are target-specific. I don't think we want to pull these in explicitly here. Different targets aren't guaranteed to implement the same types of outlining.

Also some of these things wouldn't make sense on other targets (e.g. NoLRSave)

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

711

clang-format

llvm/lib/CodeGen/MachineOutliner.cpp

78 ↗

(On Diff #257136)

Do you have to include string here?

1151 ↗

(On Diff #257136)

Is it possible to use a Twine here?

http://llvm.org/docs/ProgrammersManual.html#the-twine-class

1152–1161 ↗

(On Diff #257136)

I think that this should be implemented in the target rather than in the generic algorithm.

1152–1161 ↗

(On Diff #257136)

I think that you want to communicate the FrameID here, not the CallConstructionID?

One outlined function can be called in more than one way. E.g. it's possible to call the same outlined function using the NoLRSave and RegSave modes.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

5559–5565

Commented out code in patch?

llvm/lib/Target/X86/X86InstrInfo.cpp

8666–8669 ↗

(On Diff #257136)

Commented out code in patch?

[AndrewLitteken]

Tests have been updated to look for the annotations.

The checking now happens on the FrameID and occurs in the Target rather than the Outliner.

[paquette]

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

6410–6411

Can we move this comment into the thunk case?

Having it above the OutlinerString is somewhat out of place IMO.

6436–6437

Can we have this be the first case to make the braces a bit nicer?

if (OF.FrameConstructionID == MachineOutlinerTailCall)
    OutlinerString = "Tail Call";
else if (OF.FrameConstructionID == MachineOutlinerThunk) {
  // .. stuff
}

llvm/test/CodeGen/AArch64/machine-outliner-function-annotate.ll

1 ↗

(On Diff #257177)

Is it possible to write this as a MIR test? In general, MIR tests are better for late passes like the outliner because they let us know exactly which instructions the outliner will work on. With IR tests, we are fragile against other changes in the code generator.

To generate MIR, you can run:

llc (all the flags you want) -stop-before=machine-outliner -simplify-mir

Then you can create a testcase with a RUN line similar to below:

llc (all the flags you want) -run-pass=machine-outliner ...

31 ↗

(On Diff #257177)

Missing newline?

[AndrewLitteken]

llvm/test/CodeGen/AArch64/machine-outliner-function-annotate.ll

1 ↗

(On Diff #257177)

I'm not sure that this works in this case, because the annotations are on the assembly code and don't show up in the MIR. Based on the docs I've found you can't output assembly from MIR.

[paquette]

llvm/test/CodeGen/AArch64/machine-outliner-function-annotate.ll

1 ↗

(On Diff #257177)

Oh, yeah, you're right. :)

To output assembly, you can use

llc (all the flags you want) -start-before=machine-outliner ...

You may also have to add -enable-machine-outliner.

This tells llc to start the pass pipeline at the point where the MachineOutliner should run. It will continue the pass pipeline through until emitting assembly.

[AndrewLitteken]

A lot of changes:

1. Split emitting the comment into its own function and only defined in AArch64ASMPrinter.cpp
2. New MIR test for function annotation
3. Support for the MIR test, so that AArchMachineFunctionInfo hasRedZone attribute is output to the MIR file and is read back in through the YAML parsing

[paquette]

I think this is a better direction than before. Also, I think having the redzone attribute in the MIR is very helpful for outliner tests in general.

I think this should be two separate patches though: one to add the attribute to the MIR, and one to add comments to the outlined functions. Reason being that

1. I think that the attribute in the MIR needs a few standalone test cases
2. This is useful functionality *outside* of the outliner which I think could allow us to simplify future outliner MIR tests

llvm/include/llvm/CodeGen/AsmPrinter.h

684

Might want a Doxygen comment here, so that this will have an entry in the AsmPrinter docs.

e.g.

/// Emits a comment for a function header.

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

134

This should have a Doxygen comment.

[AndrewLitteken]

What's the best way to split these up? The test for the outlined comments rely on the RedZone Function Info.

[AndrewLitteken]

I updated for Doxygen comments, I created a new revision D78173 that implements the MIR patch, but haven't removed it here yet since the MIR test is dependent on that being in place.

[AndrewLitteken]

Updating the diff after merging the noRedZone function info patch

[paquette]

Added some comments on the testcase.

It'd be nice to just remove the IR entirely if possible, or at the very least simplify it down to a minimal form. Simple testcases are easier to fix +understand when they inevitably break someday. :)

llvm/test/CodeGen/AArch64/machine-outliner-function-annotate.mir

16–19

This can be deleted, I think.

22–26

If you replace all instances of bb.0.entry with bb.0, you can delete the IR code here.

e.g. replace it with

define void @a() { ret void }

31

This can probably just be define void?

In general, 99% of the IR isn't necessary in MIR tests. Only things that are directly referenced in the MIR are necessary (variables, function names, etc.)

33–36

This IR code can be deleted as well, if you replace all instances of bb.0.entry in the MIR.

40

This can be deleted

42

This isn't used anywhere, so it can be deleted.

56–59

Can you delete this?

67

You can replace (store 8 into %stack.2) with (store 8) to simplify the MIR a bit.

76

Is it possible to get this testcase to work without a BL?

If it is, then I think you can delete all of the IR and leave only the MIR.

77

Replace (store 4 into %ir.ptr) with (store 4)

(There are a few other places with code like this which can similarly be simplified.)

94–97

Can you delete this?

[AndrewLitteken]

Simplified version of the new mir test

[paquette]

LGTM

llvm/include/llvm/CodeGen/AsmPrinter.h

private:
   // Assign a unique ID to this machine instruction.
   mutable const MachineInstr *LastMI = nullptr;
   mutable unsigned LastFn = 0;
   mutable unsigned Counter = ~0U;
 
   /// This method emits the header for the current function.
   virtual void emitFunctionHeader();
 
+  /// This method emits a comment next to header for the current function.
+  virtual void emitFunctionHeaderComment();
+
   /// Emit a blob of inline asm to the output streamer.
   void
   emitInlineAsm(StringRef Str, const MCSubtargetInfo &STI,
                 const MCTargetOptions &MCOptions,
                 const MDNode *LocMDNode = nullptr,
                 InlineAsm::AsmDialect AsmDialect = InlineAsm::AD_ATT) const;
 
   /// This method formats and emits the specified machine instruction that is an

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

 /// Emit the directive and value for debug thread local expression
 ///
 /// \p Value - The value to emit.
 /// \p Size - The size of the integer (in bytes) to emit.
 void AsmPrinter::emitDebugValue(const MCExpr *Value, unsigned Size) const {
   OutStreamer->emitValue(Value, Size);
 }
 
+void AsmPrinter::emitFunctionHeaderComment() {}
+
 /// EmitFunctionHeader - This method emits the header for the current
 /// function.
 void AsmPrinter::emitFunctionHeader() {
   const Function &F = MF->getFunction();
 
   if (isVerbose())
     OutStreamer->GetCommentOS()
         << "-- Begin function "
if (MAI->hasDotTypeDotSizeDirective())
     OutStreamer->emitSymbolAttribute(CurrentFnSym, MCSA_ELF_TypeFunction);
 
   if (F.hasFnAttribute(Attribute::Cold))
     OutStreamer->emitSymbolAttribute(CurrentFnSym, MCSA_Cold);
 
   if (isVerbose()) {
     F.printAsOperand(OutStreamer->GetCommentOS(),
                    /*PrintType=*/false, F.getParent());
+    emitFunctionHeaderComment();
     OutStreamer->GetCommentOS() << '\n';
   }
 
   // Emit the prefix data.
   if (F.hasPrefixData()) {
     if (MAI->hasSubsectionsViaSymbols()) {
       // Preserving prefix data on platforms which use subsections-via-symbols
       // is a bit tricky. Here we introduce a symbol for the prefix data
       // and use the .alt_entry attribute to mark the function's real entry point
       // as an alternative entry point to the prefix-data symbol.

llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp

public:
 
   /// tblgen'erated driver function for lowering simple MI->MC
   /// pseudo instructions.
   bool emitPseudoExpansionLowering(MCStreamer &OutStreamer,
                                    const MachineInstr *MI);
 
   void emitInstruction(const MachineInstr *MI) override;
 
+  void emitFunctionHeaderComment() override;
+
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AsmPrinter::getAnalysisUsage(AU);
     AU.setPreservesAll();
   }
 
   bool runOnMachineFunction(MachineFunction &MF) override {
     AArch64FI = MF.getInfo<AArch64FunctionInfo>();
     STI = static_cast<const AArch64Subtarget*>(&MF.getSubtarget());
void AArch64AsmPrinter::emitStartOfAsmFile(Module &M) {
   OutStreamer->emitInt32(4);     // data size
   OutStreamer->emitInt32(Flags); // data
   OutStreamer->emitInt32(0);     // pad
 
   OutStreamer->endSection(Nt);
   OutStreamer->SwitchSection(Cur);
 }
 
+void AArch64AsmPrinter::emitFunctionHeaderComment() {
+  const AArch64FunctionInfo *FI = MF->getInfo<AArch64FunctionInfo>();
+  Optional<std::string> OutlinerString = FI->getOutliningStyle();
+  if (OutlinerString != None)
+    OutStreamer->GetCommentOS() << ' ' << OutlinerString;
+}
+
 void AArch64AsmPrinter::LowerPATCHABLE_FUNCTION_ENTER(const MachineInstr &MI)
 {
   const Function &F = MF->getFunction();
   if (F.hasFnAttribute("patchable-function-entry")) {
     unsigned Num;
     if (F.getFnAttribute("patchable-function-entry")
             .getValueAsString()
             .getAsInteger(10, Num))

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp

 /// I2 --> BL OUTLINED_FUNCTION       I1
 /// I3     Restore LR                 I2
 ///                                   I3
 ///                                   RET
 ///
 /// * Call construction overhead: 3 (save + BL + restore)
 /// * Frame construction overhead: 1 (ret)
 /// * Requires stack fixups? No
 enum MachineOutlinerClass {
   MachineOutlinerDefault,  /// Emit a save, restore, call, and return.
   MachineOutlinerTailCall, /// Only emit a branch.
   MachineOutlinerNoLRSave, /// Emit a call and return.
   MachineOutlinerThunk,    /// Emit a call and tail-call.
   MachineOutlinerRegSave   /// Same as default, but save to a register.
 };
 
 enum MachineOutlinerMBBFlags {
   LRUnavailableSomewhere = 0x2,
   HasCalls = 0x4,
   UnsafeRegsDead = 0x8
 };
 
 unsigned
if (Subtarget.hasV8_3aOps() && MBBAUT != MBB.end() &&
                                                     : AArch64::AUTIBSP))
           .setMIFlag(MachineInstr::FrameDestroy);
     }
   }
 }
 
 void AArch64InstrInfo::buildOutlinedFrame(
     MachineBasicBlock &MBB, MachineFunction &MF,
     const outliner::OutlinedFunction &OF) const {
+
+  AArch64FunctionInfo *FI = MF.getInfo<AArch64FunctionInfo>();
+
+  if (OF.FrameConstructionID == MachineOutlinerTailCall)
+    FI->setOutliningStyle("Tail Call");
+  else if (OF.FrameConstructionID == MachineOutlinerThunk) {
     // For thunk outlining, rewrite the last instruction from a call to a
     // tail-call.
-  if (OF.FrameConstructionID == MachineOutlinerThunk) {
     MachineInstr *Call = &*--MBB.instr_end();
     unsigned TailOpcode;
     if (Call->getOpcode() == AArch64::BL) {
       TailOpcode = AArch64::TCRETURNdi;
     } else {
       assert(Call->getOpcode() == AArch64::BLR);
       TailOpcode = AArch64::TCRETURNriALL;
     }
     MachineInstr *TC = BuildMI(MF, DebugLoc(), get(TailOpcode))
                            .add(Call->getOperand(0))
                            .addImm(0);
     MBB.insert(MBB.end(), TC);
     Call->eraseFromParent();
+
+    FI->setOutliningStyle("Thunk");
   }
 
   bool IsLeafFunction = true;
 
   // Is there a call in the outlined range?
   auto IsNonTailCall = [](const MachineInstr &MI) {
     return MI.isCall() && !MI.isReturn();
   };
 
   if (std::any_of(MBB.instr_begin(), MBB.instr_end(), IsNonTailCall)) {
     // Fix up the instructions in the range, since we're going to modify the
     // stack.
void AArch64InstrInfo::buildOutlinedFrame(
   // It's not a tail call, so we have to insert the return ourselves.
   MachineInstr *ret = BuildMI(MF, DebugLoc(), get(AArch64::RET))
                           .addReg(AArch64::LR, RegState::Undef);
   MBB.insert(MBB.end(), ret);
 
   signOutlinedFunction(MF, MBB, ShouldSignReturnAddr,
                        ShouldSignReturnAddrWithAKey);
 
+  FI->setOutliningStyle("Function");
+
   // Did we have to modify the stack by saving the link register?
   if (OF.FrameConstructionID != MachineOutlinerDefault)
     return;
 
   // We modified the stack.
   // Walk over the basic block and fix up all the stack accesses.
   fixupPostOutline(MBB);
 }

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h

class AArch64FunctionInfo final : public MachineFunctionInfo {
   /// that must be forwarded to every musttail call.
   SmallVector<ForwardedRegister, 1> ForwardedMustTailRegParms;
 
   // Offset from SP-at-entry to the tagged base pointer.
   // Tagged base pointer is set up to point to the first (lowest address) tagged
   // stack slot.
   unsigned TaggedBasePointerOffset = 0;
 
+  /// OutliningStyle denotes, if a function was outined, how it was outlined,
+  /// e.g. Tail Call, Thunk, or Function if none apply.
+  Optional<std::string> OutliningStyle;
+
 public:
   AArch64FunctionInfo() = default;
 
   explicit AArch64FunctionInfo(MachineFunction &MF) {
     (void)MF;
 
     // If we already know that the function doesn't have a redzone, set
     // HasRedZone here.
void setCalleeSaveStackHasFreeSpace(bool s) {
     CalleeSaveStackHasFreeSpace = s;
   }
   bool isSplitCSR() const { return IsSplitCSR; }
   void setIsSplitCSR(bool s) { IsSplitCSR = s; }
 
   void setLocalStackSize(uint64_t Size) { LocalStackSize = Size; }
   uint64_t getLocalStackSize() const { return LocalStackSize; }
 
+  void setOutliningStyle(std::string Style) { OutliningStyle = Style; }
+  Optional<std::string> getOutliningStyle() const { return OutliningStyle; }
+
   void setCalleeSavedStackSize(unsigned Size) {
     CalleeSavedStackSize = Size;
     HasCalleeSavedStackSize = true;
   }
 
   // When CalleeSavedStackSize has not been set (for example when
   // some MachineIR pass is run in isolation), then recalculate
   // the CalleeSavedStackSize directly from the CalleeSavedInfo.

llvm/test/CodeGen/AArch64/machine-outliner-function-annotate.mir

+# RUN: llc -mtriple=aarch64--- -start-before=machine-outliner -enable-machine-outliner %s -o - | FileCheck %s
+
+# Check that a non tail called or thunk function is annotated properly with
+# only "Function"
+
+# CHECK-LABEL: OUTLINED_FUNCTION_0:
+# CHECK-SAME: // @OUTLINED_FUNCTION_0 Function
+# CHECK:      mov     w0, #1
+# CHECK-NEXT: mov     w1, #2
+# CHECK-NEXT: mov     w2, #3
+# CHECK-NEXT: mov     w3, #4
+# CHECK-NEXT: ret
+
+---
+name:            a
+alignment:       4
+tracksRegLiveness: true
+machineFunctionInfo:
+  hasRedZone:      false
+body:             |
+  bb.0:
+    frame-setup CFI_INSTRUCTION def_cfa_offset 32
+    frame-setup CFI_INSTRUCTION offset $w19, -8
+    frame-setup CFI_INSTRUCTION offset $w30, -16
+    $w0 = MOVZWi 1, 0
+    $w1 = MOVZWi 2, 0
+    $w2 = MOVZWi 3, 0
+    $w3 = MOVZWi 4, 0
+    renamable $w19 = MOVZWi 2, 0
+    RET undef $lr
+
+...
+---
+name:            b
+alignment:       4
+tracksRegLiveness: true
+machineFunctionInfo:
+  hasRedZone:      false
+body:             |
+  bb.0:
+    frame-setup CFI_INSTRUCTION def_cfa_offset 32
+    frame-setup CFI_INSTRUCTION offset $w19, -8
+    frame-setup CFI_INSTRUCTION offset $w30, -16
+    $w0 = MOVZWi 1, 0
+    $w1 = MOVZWi 2, 0
+    $w2 = MOVZWi 3, 0
+    $w3 = MOVZWi 4, 0
+    renamable $w19 = MOVZWi 1, 0
+    RET undef $lr
+
+...

llvm/test/CodeGen/AArch64/machine-outliner-tail.ll

 ; RUN: llc -verify-machineinstrs -enable-machine-outliner -mtriple=aarch64-linux-gnu < %s | FileCheck %s
 
-; CHECK: OUTLINED_FUNCTION_0:
+; CHECK-LABEL: OUTLINED_FUNCTION_0:
+; CHECK-SAME: // @OUTLINED_FUNCTION_0 Tail Call
 ; CHECK:      mov     w0, #1
 ; CHECK-NEXT: mov     w1, #2
 ; CHECK-NEXT: mov     w2, #3
 ; CHECK-NEXT: mov     w3, #4
 ; CHECK-NEXT: b       z
 
 define void @a() {
 entry:

llvm/test/CodeGen/AArch64/machine-outliner-thunk.ll

 ; CHECK-NEXT:    ret
 entry:
   %call = tail call i32 %fptr(i32 1, i32 2, i32 3, i32 4)
   %add = add nsw i32 %call, 88
   ret i32 %add
 }
 
 ; CHECK: [[OUTLINED_INDIRECT]]:
+; CHECK-SAME: // @[[OUTLINED_INDIRECT]] Thunk
 ; CHECK:        // %bb.0:
 ; CHECK-NEXT:   mov     x8, x0
 ; CHECK-NEXT:   mov     w0, #1
 ; CHECK-NEXT:   mov     w1, #2
 ; CHECK-NEXT:   mov     w2, #3
 ; CHECK-NEXT:   mov     w3, #4
 ; CHECK-NEXT:   br      x8
 
 ; CHECK: [[OUTLINED_DIRECT]]:
+; CHECK-SAME: // @[[OUTLINED_DIRECT]] Thunk
 ; CHECK:       // %bb.0:
 ; CHECK-NEXT:    mov     w0, #1
 ; CHECK-NEXT:    mov     w1, #2
 ; CHECK-NEXT:    mov     w2, #3
 ; CHECK-NEXT:    mov     w3, #4
 ; CHECK-NEXT:    b       thunk_called_fn