Sorry I could not provide bitcode file since it is related to IP issue.

Hi Florian,

Testing Time: 128.22s

Expected Passes    : 36105
Expected Failures  : 163
Unsupported Tests  : 336

I have made minor changes in test case llvm/test/CodeGen/AArch64/machine-outliner-iterative.mir.

In D71027#1925639, @paquette wrote:

LGTM!

Thank you all for your review and suggestions!

I have not made changes in swift side since the changes in LLVM have to go in first.

All the llvm-lit tests passed without any unexpected fails.

I have updated the diffs. Now when you compare Diff 248212 with Diff 248301, you will see they are the same.

In D71217#1906007, @aemerson wrote:

Hi, I just tried out this patch locally and I'm seeing failures running the tests:

Failing Tests (3):
    LLVM :: CodeGen/AArch64/machine-outliner-cfi.mir
    LLVM :: CodeGen/AArch64/machine-outliner-noreturn-save-lr.mir
    LLVM :: CodeGen/AArch64/machine-outliner-side-effect.mir

In D71217#1906007, @aemerson wrote:

Hi, I just tried out this patch locally and I'm seeing failures running the tests:

Failing Tests (3):
    LLVM :: CodeGen/AArch64/machine-outliner-cfi.mir
    LLVM :: CodeGen/AArch64/machine-outliner-noreturn-save-lr.mir
    LLVM :: CodeGen/AArch64/machine-outliner-side-effect.mir

Simplified the test case.

rebase master

In D71217#1905675, @paquette wrote:

The changes to machine-outliner-noreturn-save-lr.mir look fine to me.

Hi Jessica,

In D71217#1904168, @paquette wrote:

LGTM!

In D71217#1903635, @paquette wrote:

The reason you weren't getting outlined functions is probably because there were attributes missing on the function.

I think that we should be able to simplify it further like this:

# RUN: llc -mtriple=aarch64 -run-pass=machine-outliner -verify-machineinstrs %s -o - | FileCheck %s

# The test checks whether the compiler updates the side effect of function @OUTLINED_FUNCTION_0 by adding the use of register x20.

--- |
  declare void @spam() local_unnamed_addr
  define void @baz() optsize minsize noredzone { ret void }
...
---
name:            baz
tracksRegLiveness: true
body:             |
  bb.0:
    liveins: $x0, $x20

    $x0 = COPY renamable $x20
    BL @spam, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit-def $sp, implicit-def $x0
    renamable $x21 = COPY $x0

    $x0 = COPY renamable $x20
    BL @spam, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit-def $sp, implicit-def $x0
    renamable $x22 = COPY $x0

    $x0 = COPY killed renamable $x20
    BL @spam, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit-def $sp, implicit-def $x0
    renamable $x3 = COPY $x0

    RET_ReallyLR

...

# CHECK: BL @OUTLINED_FUNCTION_0, {{.*}}, implicit $x20, {{.*}}

Thank you Jessica for quick feedback. I have updated the test case based on your advice.

Hi Jessica,

In D71219#1891857, @steven_wu wrote:

In D71219#1890444, @jinlin wrote:

In D71219#1791936, @steven_wu wrote:

1. I will add a llvm-link tests for combining objc and swift bitcode.
2. What do you mean by "codegen tests for correct value in the object file"? There is no "object file" in the picture. Are you asking for a test that checks the value in the output of llvm-link?

I think the decision was to break apart the current "Objective-C Garbage Collection" metadata. You need codegen test to test that OBJC_IMAGE_INFO is generated correctly after breaking apart the value.

I have question regarding this. Let's assume we have one assembly file from Objective-C and another one from Swift.

Objective-C assembly:
L_OBJC_IMAGE_INFO:
.long 0
.long 64

Swift assembly:

L_OBJC_IMAGE_INFO:
.long 0
.long 83953472

Let's assume that the llvm-link can link Swift bitcode and Objective-C bit successfully and the llc can generate the assembly for the merged output bitcode.

In the final assembly file, what value do you expect for L_OBJC_IMAGE_INFO?

You can easily test that out with ld64. It depends on the bits in the current garbage collection fields. Most of the fields are useless now (only there for GC which is obsoleted), that is why the module flag uses Override. Some bits in there are taking Max, like OBJC_IMAGE_HAS_CATEGORY_CLASS_PROPERTIES. Swift version should be taking Error since mismatch is a failure, unless it is not set.

In D71219#1891857, @steven_wu wrote:

In D71219#1890444, @jinlin wrote:

In D71219#1791936, @steven_wu wrote:

1. I will add a llvm-link tests for combining objc and swift bitcode.
2. What do you mean by "codegen tests for correct value in the object file"? There is no "object file" in the picture. Are you asking for a test that checks the value in the output of llvm-link?

I think the decision was to break apart the current "Objective-C Garbage Collection" metadata. You need codegen test to test that OBJC_IMAGE_INFO is generated correctly after breaking apart the value.

In D71219#1791936, @steven_wu wrote:

1. I will add a llvm-link tests for combining objc and swift bitcode.
2. What do you mean by "codegen tests for correct value in the object file"? There is no "object file" in the picture. Are you asking for a test that checks the value in the output of llvm-link?

I think the decision was to break apart the current "Objective-C Garbage Collection" metadata. You need codegen test to test that OBJC_IMAGE_INFO is generated correctly after breaking apart the value.

Update with llvm-link test case.

Update the test from IR file to MIR file.

Updated test case.

Also, I guess it would also be good to add a testcase that ensures that a register is not added as implicit when it's undefined in the range.

In D71217#1875226, @paquette wrote:

The outlined function in this test case does not serve the purpose since there are no live in registers. I did check this test case before. If I change the arguments 1, 2, 3, 4 to be incoming registers, the machine outliner won't kick in.

Maybe I'm misunderstanding something here?

(1) Locally, with the patch, adding

liveins: $w0, $w1, $w2, $w3, $w4

does not change the outliner's behaviour.

(2) If I understand correctly, what you are trying to test here is that implicit $xN is added to the outlined call when $xN is not undefined in the outlined range.

Without this patch, this testcase produces

BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $w0, implicit-def $w1, implicit-def $w2, implicit-def $w3

With the patch, it adds implicit $wzr, implicit $w4 at the end:

BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $w3, implicit-def $w2, implicit-def $w1, implicit-def $w0, implicit-def $lr, implicit-def $w3, implicit-def $w2, implicit-def $w1, implicit-def $w0, implicit $sp, implicit $wzr, implicit $w4

It's kind of weird that the implicit defs are duplicated though. ($lr appears twice in both cases). I'd expect it to be

BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit-def $w0, implicit-def $w1, implicit-def $w2, implicit-def $w3, implicit $sp, implicit $wzr, implicit $w4

Also, I guess it would also be good to add a testcase that ensures that a register is not added as implicit when it's undefined in the range.

The reason that the implicit defs are duplicate is because the compiler traverses the instructions in the reverse order and update the side effect of the new call instruction on the fly.
So the def reg set is introduced to avoid the redundant register.

In D71217#1875153, @paquette wrote:

I think a MIR testcase would be simpler here. Something like this should work, no?

# NOTE: Assertions have been autogenerated by utils/update_mir_test_checks.py
# RUN: llc -mtriple=aarch64-apple-darwin -run-pass=machine-outliner -verify-machineinstrs  %s -o - | FileCheck %s
--- |
  define void @foo() noredzone {ret void}
...
---

name:            foo
tracksRegLiveness: true
body:             |
  ; CHECK-LABEL: name: foo
  ; CHECK: bb.0:
  ; CHECK:   successors: %bb.1(0x80000000)
  ; CHECK:   liveins: $w4
  ; CHECK:   BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $w3, implicit-def $w2, implicit-def $w1, implicit-def $w0, implicit-def $lr, implicit-def $w3, implicit-def $w2, implicit-def $w1, implicit-def $w0, implicit $sp, implicit $wzr, implicit $w4
  ; CHECK: bb.1:
  ; CHECK:   successors: %bb.2(0x80000000)
  ; CHECK:   liveins: $w4
  ; CHECK:   BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $w3, implicit-def $w2, implicit-def $w1, implicit-def $w0, implicit-def $lr, implicit-def $w3, implicit-def $w2, implicit-def $w1, implicit-def $w0, implicit $sp, implicit $wzr, implicit $w4
  ; CHECK: bb.2:
  ; CHECK:   successors: %bb.3(0x80000000)
  ; CHECK:   liveins: $w4
  ; CHECK:   BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $w3, implicit-def $w2, implicit-def $w1, implicit-def $w0, implicit-def $lr, implicit-def $w3, implicit-def $w2, implicit-def $w1, implicit-def $w0, implicit $sp, implicit $wzr, implicit $w4
  ; CHECK: bb.3:
  ; CHECK:   liveins: $w4
  ; CHECK:   RET_ReallyLR
  bb.0:
  liveins: $w4
    $w0 = ORRWri $wzr, 1
    $w1 = ORRWri $wzr, 2
    $w2 = ORRWri $wzr, 3
    $w3 = ORRWri $w4, 4
  bb.1:
  liveins: $w4
    $w0 = ORRWri $wzr, 1
    $w1 = ORRWri $wzr, 2
    $w2 = ORRWri $wzr, 3
    $w3 = ORRWri $w4, 4
  bb.2:
    liveins: $w4
    $w0 = ORRWri $wzr, 1
    $w1 = ORRWri $wzr, 2
    $w2 = ORRWri $wzr, 3
    $w3 = ORRWri $w4, 4
  bb.3:
    liveins: $w4
    RET_ReallyLR

More minor changes.

Fix typos in the comments.

I have updated the code based on reviewers' feedback and added one test case. Thanks.

In D71027#1775926, @tellenbach wrote:

In D71027#1775515, @jinlin wrote:

Sure. I have updated the patch. Please note that the repeating machine outliner may not work correctly without fixing the logic at line 1265.

Thanks! I've commented on D69446 to see if the revision is still active. So I suggest giving this some more time.

In D71219#1776158, @steven_wu wrote:

Breaking part the meta data sounds like a good idea. You will probably need to combine the value in the backend to generate the field in the object file.

Also, please add test case. If you go down the route of splitting the value, you need:

• codegen tests for correct value in the object file
• llvm-link tests for combining objc and swift bitcode
• auto upgrade test if you want to support the old bitcode

In D71217#1775913, @tellenbach wrote:

As @lebedev.ri already said, is it possible to add a test for this? I know it is probably covered in your test for D71027 but an independent test would be great!

In D71027#1774949, @tellenbach wrote:

Okay, thanks for clarifying.

In D71027#1772142, @jinlin wrote:

In D71027, I have also fixed incorrect logic in maintaining the side-effect information for outlined function at line 1295. This bug shows up during repeated machine outliner.

Would it be possible to separate both patches? The fix can then be reviewed independently from repeated outlining itself.

In D71027#1772074, @tellenbach wrote:

Hi,

D69446 seems to introduce something similar. Are these patches related or is this just coincidence?