This is an archive of the discontinued LLVM Phabricator instance.

Adding Comment Annotations to Outlined Functions
ClosedPublic

Authored by AndrewLitteken on Apr 13 2020, 4:02 PM.

Details

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.

Diff Detail

Event Timeline

AndrewLitteken created this revision.Apr 13 2020, 4:02 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2020, 4:02 PM

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)
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 marked 6 inline comments as done.

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 added inline comments.Apr 13 2020, 7:22 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
6375–6376

Can we move this comment into the thunk case?

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

6401–6402

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 marked an inline comment as done.Apr 13 2020, 9:37 PM
AndrewLitteken added inline comments.
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 added inline comments.Apr 14 2020, 10:47 AM
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 marked 3 inline comments as done.

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

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
680

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.

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

AndrewLitteken marked 2 inline comments as done.

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.

Updating the diff after merging the noRedZone function info patch

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?

Simplified version of the new mir test

This revision is now accepted and ready to land.Apr 20 2020, 1:02 PM
This revision was automatically updated to reflect the committed changes.