This is an archive of the discontinued LLVM Phabricator instance.

Outline CFI Instructions in Tail Calls
ClosedPublic

Authored by AndrewLitteken on Apr 9 2020, 7:45 PM.

Details

Reviewers
paquette
thegameg
Summary

This patch outlines CFI Instructions if the outlined function is part of a tail call

Diff Detail

Event Timeline

AndrewLitteken created this revision.Apr 9 2020, 7:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 9 2020, 7:45 PM
paquette added inline comments.Apr 9 2020, 8:34 PM
llvm/lib/CodeGen/MachineOutliner.cpp
1222

Why is this out here?

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5867–5869

Typically in LLVM, we prefer omitting braces on ifs with a single line of code.

I think running clang-format should handle this automatically.

Also variable names are written with a capital letter in LLVM. (see: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly)

6193–6198

Comment is outdated now

llvm/test/CodeGen/AArch64/machine-outliner-cfi.mir
6

This testcase is the same as the new one you're adding, right?

I don't think we need both testcases if so. I think it would be better to just keep this one and not add a new, identical test.

AndrewLitteken marked an inline comment as done.Apr 9 2020, 8:51 PM
AndrewLitteken added inline comments.
llvm/test/CodeGen/AArch64/machine-outliner-cfi.mir
6

This is the old test so that it actually shows this. The previous test could outline the CFI instruction, this one should not. The comments are the same because I was copy pasting the file to get the testing framework correct.

paquette added inline comments.Apr 9 2020, 9:47 PM
llvm/test/CodeGen/AArch64/machine-outliner-cfi-tail.mir
5–7

Is this comment accurate? This test is to show that we do outline CFI instructions in tail calls, right?

llvm/test/CodeGen/AArch64/machine-outliner-cfi.mir
6

Ah now I see!

Francis, you had some concerns about unwinding here. Can you take a look as well?

llvm/lib/CodeGen/MachineOutliner.cpp
1170–1171

It looks like you copy over every CFI from the (first) original function into the outlined function here.

However, I don't think that there are any guarantees that *every* CFI instruction from the original function is actually present in the outlined sequence right now.

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5865

I don't think you have to check any_of. They should all contain the same instructions.

6067–6068

We probably also want to check that the candidate has all of the original function's CFI instructions

AndrewLitteken set the repository for this revision to rG LLVM Github Monorepo.

Taking more care to include all CFI instructions and general formatting, also making sure all the tests pass

paquette added inline comments.Apr 10 2020, 5:04 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
50

It looks like std::set isn't used in the patch anywhere, so you probably don't need this.

51

You shouldn't have to include vector here. It is already included by something else.

5866

Can this be a SmallVector?

5867–5870

I think comment is kind of confusing.

  • Substring doesn't really imply equality. It would make more sense to say "every candidate has equivalent instructions".
  • "size matches" doesn't way what is being referred to with respect to size. You could say "the number of CFI instructions found is equal to the number in the containing function's frame" or something similar.
  • I think we should add some reasoning for why we don't want to allow candidates which do not include all of the CFIs.
5867–5872

I think we also have to check that the other Candidate's functions have the same number of CFI instructions as well.

e.g. we shouldn't have

foo:
  cfi 0
  cfi 1
  cfi 2
  ...
  ret

bar:
  cfi 1
  cfi 2
  ...
  ret

be outlined to

foo:
  cfi 0
  b outlined

bar:
  b outlined

I think that this could happen if for whatever reason bar ends up containing the first Candidate.

5868–5869

I think it would be good to express the intent of this loop here, rather than in the comment below.

e.g.

// To safely outline CFI instructions, we must outline every CFI instruction in the function.
// If CFI instructions are present in the candidate, store them in a vector. This can be used to check if we have collected all of the CFI instructions.
6194–6196

We don't fix up the offsets, so maybe we should just remove the first sentence in this comment.

paquette added inline comments.Apr 10 2020, 7:22 PM
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5867

s/colllect/collect/

5871

Missing period at the end of sentence. (https://llvm.org/docs/CodingStandards.html#commenting)

5873

Use outliner::Candidate &C?

5876

Pull vector out of the loop and use clear()? (See: http://llvm.org/docs/ProgrammersManual.html#vector)

5878–5884

I don't think you have to do this for every candidate.

The part you have to do for every candidate is checking the number of CFI instructions in each function frame. This should be sufficient, because we know every instruction in every candidate is the same.

E.g. after collecting the CFI instructions in the first candidate, you can do something like this:

if (HasCFI) {
  for (outliner::Candidate &C : RepeatedSequenceLocs) {
    if (C.getMF()->getFrameInstructions().size() != CFIInstructionsPresent.size())
      return outliner::OutlinedFunction();
  }
}

or even

if (HasCFI && any_of(RepeatedSequenceLocs, ...))
  return outliner::OutlinedFunction()

Actually, is it even necessary to collect the CFI instructions? Since all we need to know is that the candidates actually contain all of them, it would be sufficient to just count how many are in the candidate versus how many are in the frame of each function, no?

AndrewLitteken marked an inline comment as done.Apr 10 2020, 7:35 PM
AndrewLitteken added inline comments.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5878–5884

I see now, I thought you reversed what you said in the first review about not having to check everything. I also agree, just finding the number should be sufficient, we shouldn't even need use a vector.

Added a new test to make sure that we do not outline CFI Instruction if the outline would not capture all the CFI instruction in a function of one of the candidates

paquette added inline comments.Apr 10 2020, 9:17 PM
llvm/lib/CodeGen/MachineOutliner.cpp
1167–1169

Can we move this out of the loop?

llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5866

s/Instruction/Instructions/
s/cadidates/candidates/

5869–5870

There are some extra spaces here which can be clang-formatted away

5881

extra space after "to"

This revision is now accepted and ready to land.Apr 10 2020, 10:09 PM

Thanks Andrew! A few coments inline and below:

  • does this need an X86 test as well? I'm not sure what the state of the outliner is there.
  • please grab std::vector<MCCFIInstruction> by const &.
  • a MachineBasicBlock::iterator is usually called MBBI in codegen.
llvm/lib/Target/AArch64/AArch64InstrInfo.cpp
5866

s/Instructionz/Instructions/

llvm/lib/Target/X86/X86InstrInfo.cpp
8686

s/Instructionz/Instructions/

llvm/test/CodeGen/AArch64/machine-outliner-cfi-tail-some.mir
23 ↗(On Diff #256742)

CHECK-NEXT might be even better here.

thegameg accepted this revision.Apr 13 2020, 10:28 AM
AndrewLitteken added a comment.EditedApr 13 2020, 12:10 PM
  • does this need an X86 test as well? I'm not sure what the state of the outliner is there.

I'm not sure, I'll try writing some analogous ones

vsk added a subscriber: vsk.Apr 24 2020, 4:57 PM

Hi all,

I'm seeing what looks like an unexpected use of $noreg when I run some of these tests with synthetic debug uses inserted into the MIR stream (with -debugify-and-strip-all-safe). This is what I see:

~/src/builds/llvm-project-master-RA (1) $ ./bin/llc -debugify-and-strip-all-safe -mtriple=aarch64 -run-pass=machine-outliner -verify-machineinstrs /Users/vsk/src/llvm-project-master/llvm/test/CodeGen/AArch64/machine-outliner-side-effect.mir -o - > bad2.mir
~/src/builds/llvm-project-master-RA (0) $ ./bin/llc -debugify-and-strip-all-safe=0 -mtriple=aarch64 -run-pass=machine-outliner -verify-machineinstrs /Users/vsk/src/llvm-project-master/llvm/test/CodeGen/AArch64/machine-outliner-side-effect.mir -o - > good2.mir
~/src/builds/llvm-project-master-RA (0) $ diff -u {good,bad}2.mir
--- good2.mir   2020-04-24 16:53:17.000000000 -0700
+++ bad2.mir    2020-04-24 16:53:08.000000000 -0700
@@ -62,11 +62,11 @@
   bb.0:
     liveins: $x0, $x20

-    BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit $x20, implicit $sp
+    BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit $noreg, implicit $sp, implicit $x20
     renamable $x21 = COPY $x0
-    BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit $x20, implicit $sp
+    BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit $noreg, implicit $sp, implicit $x20
     renamable $x22 = COPY $x0
-    BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit $x20, implicit $sp
+    BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit $noreg, implicit $sp, implicit $x20
     renamable $x3 = COPY $x0
     RET_ReallyLR

Not sure if that's something to worry about (or even whether this patch causes the difference), just thought I'd report the difference. Generally we don't expect tests to fail when -debugify-and-strip-all-safe is added, but there are exceptions of course.

I think the issue is actually introduced by this patch: https://reviews.llvm.org/D78173

After going through and reverting recent outliner commits, I don't think this change is related to any of them.

Commits:
1a78b0bd3829381e7be627b459c22083bf4671d4
1488bef8fc916fb5b563122bf64b949bb5c16464
8d5024f7fe721513c4332048e6836cd408581a61
66037b84cf5e2d432801fd0a3a0f2921c0860af3