This patch outlines CFI Instructions if the outlined function is part of a tail call
Diff Detail
Event Timeline
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) | |
6195–6200 | 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. |
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. |
Francis, you had some concerns about unwinding here. Can you take a look as well?
llvm/lib/CodeGen/MachineOutliner.cpp | ||
---|---|---|
1173–1174 | 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. | |
6069–6070 | We probably also want to check that the candidate has all of the original function's CFI instructions |
Taking more care to include all CFI instructions and general formatting, also making sure all the tests pass
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.
| |
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. | |
6196–6198 | We don't fix up the offsets, so maybe we should just remove the first sentence in this comment. |
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? |
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
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 | CHECK-NEXT might be even better here. |
- 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
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
Can we move this out of the loop?