This is an archive of the discontinued LLVM Phabricator instance.

Differential D112911

Fix the side effect of outlined function when the register is implicit use and implicit-def in the same instruction.
ClosedPublic

Authored by DianQK on Nov 1 2021, 2:01 AM.

Details

Reviewers
paquette
jinlin
Commits
rG1e9fa0b12a76: Fix the side effect of outlined function when the register is implicit use and…
Summary

This is the diff associated with D95267: Add the use of register r for outlined function when register r is live in and defined later., and we need to mark $x0 as live whether or not $x0 is dead.

The compiler also needs to mark register $x0 as live in for the following case.

$x1 = ADDXri $sp, 16, 0
BL @spam, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def $x0

This change fixes an issue where the wrong registers were used when -machine-outliner-reruns>0.
As an example:

typedef struct {
    double v1;
    double v2;
} D16;

typedef struct {
    D16 v1;
    D16 v2;
} D32;

typedef long long LL8;
typedef struct {
    long long v1;
    long long v2;
} LL16;
typedef struct {
    LL16 v1;
    LL16 v2;
} LL32;

typedef struct {
    LL32 v1;
    LL32 v2;
} LL64;

LL8 needx0(LL8 v0, LL8 v1);

void bar(LL64 v1, LL32 v2, LL16 v3, LL32 v4, LL8 v5, D16 v6, D16 v7, D16 v8);

LL8 foo(LL8 v0, LL64 v1, LL32 v2, LL16 v3, LL32 v4, LL8 v5, D16 v6, D16 v7, D16 v8)
{
  LL8 result = needx0(v0, 0);
  bar(v1, v2, v3, v4, v5, v6, v7, v8);
  return result + 1;
}

As you can see from the foo function, we should not modify the value of x0 until we call needx0.
This code is compiled to give the following instruction MIR code.

$sp = frame-setup SUBXri $sp, 256, 0
frame-setup STPDi killed $d13, killed $d12, $sp, 16
frame-setup STPDi killed $d11, killed $d10, $sp, 18
frame-setup STPDi killed $d9, killed $d8, $sp, 20

frame-setup STPXi killed $x26, killed $x25, $sp, 22
frame-setup STPXi killed $x24, killed $x23, $sp, 24
frame-setup STPXi killed $x22, killed $x21, $sp, 26
frame-setup STPXi killed $x20, killed $x19, $sp, 28
...
$x1 = MOVZXi 0, 0
BL @needx0, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit $x1, implicit-def $sp, implicit-def $x0
...

Since there are some other instruction sequences that duplicate foo, after the first execution of Machine Outliner you will get:

$sp = frame-setup SUBXri $sp, 256, 0
frame-setup STPDi killed $d13, killed $d12, $sp, 16
frame-setup STPDi killed $d11, killed $d10, $sp, 18
frame-setup STPDi killed $d9, killed $d8, $sp, 20

$x7 = ORRXrs $xzr, $lr, 0
BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit $sp, implicit $xzr, implicit $x7, implicit $x19, implicit $x20, implicit $x21, implicit $x22, implicit $x23, implicit $x24, implicit $x25, implicit $x26
$lr = ORRXrs $xzr, $x7, 0
...
BL @OUTLINED_FUNCTION_1, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit-def $x1, implicit $sp
...

For the first time we outlined the following sequence:

frame-setup STPXi killed $x26, killed $x25, $sp, 22
frame-setup STPXi killed $x24, killed $x23, $sp, 24
frame-setup STPXi killed $x22, killed $x21, $sp, 26
frame-setup STPXi killed $x20, killed $x19, $sp, 28

and

$x1 = MOVZXi 0, 0
BL @needx0, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit $x1, implicit-def $sp, implicit-def $x0

When we execute the outline again, we will get:

$x0 = ORRXrs $xzr, $lr, 0 <---- here
BL @OUTLINED_FUNCTION_2_0, implicit-def $lr, implicit $sp, implicit-def $sp, implicit-def $lr, implicit $sp, implicit $xzr, implicit $d8, implicit $d9, implicit $d10, implicit $d11, implicit $d12, implicit $d13, implicit $x0
$lr = ORRXrs $xzr, $x0, 0

$x7 = ORRXrs $xzr, $lr, 0
BL @OUTLINED_FUNCTION_0, implicit-def $lr, implicit $sp, implicit-def $lr, implicit $sp, implicit $xzr, implicit $x7, implicit $x19, implicit $x20, implicit $x21, implicit $x22, implicit $x23, implicit $x24, implicit $x25, implicit $x26
$lr = ORRXrs $xzr, $x7, 0
...
BL @OUTLINED_FUNCTION_1, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit-def $x1, implicit $sp

When calling OUTLINED_FUNCTION_2_0, we used x0 to save the lr register.
The reason for the above error appears to be that:

BL @OUTLINED_FUNCTION_1, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit-def $x1, implicit $sp

should be:

BL @OUTLINED_FUNCTION_1, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit-def $x1, implicit $sp, implicit $x0

When processing the same instruction with both implicit-def $x0 and implicit $x0 we should keep implicit $x0.
A reproducible demo is available at: https://github.com/DianQK/reproduce_outlined_function_use_live_x0.

Diff Detail

Event Timeline

DianQK created this revision.Nov 1 2021, 2:01 AM
Herald added a subscriber: hiraditya. · View Herald TranscriptNov 1 2021, 2:01 AM
DianQK requested review of this revision.Nov 1 2021, 2:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2021, 2:01 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
DianQK added reviewers: paquette, jinlin.Nov 1 2021, 2:05 AM
DianQK edited the summary of this revision. (Show Details)Nov 1 2021, 2:16 AM
Herald added a subscriber: kristof.beyls. · View Herald TranscriptNov 1 2021, 2:17 AM
DianQK edited the summary of this revision. (Show Details)Nov 1 2021, 2:41 AM
DianQK edited the summary of this revision. (Show Details)Nov 1 2021, 2:44 AM
Harbormaster completed remote builds in B131698: Diff 383739.Nov 1 2021, 2:46 AM
DianQK updated this revision to Diff 383992.Nov 2 2021, 12:56 AM

remove !MOP.isDead() && and machine-outliner-side-effect-3.mir

DianQK edited the summary of this revision. (Show Details)Nov 2 2021, 12:57 AM
Harbormaster completed remote builds in B131900: Diff 383992.Nov 2 2021, 1:40 AM
DianQK updated this revision to Diff 384023.Nov 2 2021, 3:10 AM

reformat

Harbormaster completed remote builds in B131922: Diff 384023.Nov 2 2021, 3:43 AM
DianQK updated this revision to Diff 384041.Nov 2 2021, 4:25 AM

rebase

Harbormaster completed remote builds in B131937: Diff 384041.Nov 2 2021, 4:57 AM
DianQK updated this revision to Diff 384669.Nov 3 2021, 11:02 PM

rebased

Harbormaster completed remote builds in B132392: Diff 384669.Nov 4 2021, 12:07 AM
DianQK edited the summary of this revision. (Show Details)Nov 6 2021, 3:17 AM
DianQK edited the summary of this revision. (Show Details)
DianQK edited the summary of this revision. (Show Details)Nov 7 2021, 1:31 AM
jinlin added a comment.EditedNov 12 2021, 4:01 PM

Here is the output of first iteration machine outliner.
llc -march=arm64 -run-pass=machine-outliner -machine-outliner-reruns=0 --simplify-mir -verify-machineinstrs build/example.mir -o build/example.outlined.mir0

The implicit use of $x0 for outlined function OUTLINED_FUNCTION_1 is missing in this case. What we should do is to check whether implict-def and implict use of $x0 are in the same instruction or not. If so, we should ignore the implict-def. So the fix does serve this purpose.

before:

$x1 = MOVZXi 0, 0
BL @needx0, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def $x0
renamable $w0 = nsw ADDWri renamable $w0, 2, 0, implicit killed $x0

After:

frame-setup CFI_INSTRUCTION offset $w29, -16 
BL @OUTLINED_FUNCTION_1, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit-def $x1, implicit $sp
renamable $x0 = nsw ADDXri killed renamable $x0, 1, 0

name: OUTLINED_FUNCTION_1

body: |

bb.0:
  liveins: $d15, $x19, $x20, $x21, $x22, $x23, $x24, $x25, $x26, $x27, $x28, $d8, $d9, $d10, $d11, $d12, $d13, $d14, $x0
 
  $x1 = MOVZXi 0, 0
  TCRETURNdi @needx0, 0, implicit $sp
jinlin added inline comments.Nov 12 2021, 4:44 PM
llvm/test/CodeGen/AArch64/machine-outliner-side-effect-2.mir
19–20

Why do you need to change this test file? With this fix, $x0 should be marked as use.

DianQK updated this revision to Diff 387013.Nov 13 2021, 3:43 AM

remove unnecessary contents

DianQK marked an inline comment as done.Nov 13 2021, 4:04 AM
In D112911#3128737, @jinlin wrote:

Here is the output of first iteration machine outliner.
llc -march=arm64 -run-pass=machine-outliner -machine-outliner-reruns=0 --simplify-mir -verify-machineinstrs build/example.mir -o build/example.outlined.mir0

The implicit use of $x0 for outlined function OUTLINED_FUNCTION_1 is missing in this case. What we should do is to check whether implict-def and implict use of $x0 are in the same instruction or not. If so, we should ignore the implict-def. So the fix does serve this purpose.

before:

$x1 = MOVZXi 0, 0
BL @needx0, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def $x0
renamable $w0 = nsw ADDWri renamable $w0, 2, 0, implicit killed $x0

After:

frame-setup CFI_INSTRUCTION offset $w29, -16 
BL @OUTLINED_FUNCTION_1, implicit-def $lr, implicit $sp, implicit-def $lr, implicit-def $sp, implicit-def $x0, implicit-def $x1, implicit $sp
renamable $x0 = nsw ADDXri killed renamable $x0, 1, 0

name: OUTLINED_FUNCTION_1

body: |

bb.0:
  liveins: $d15, $x19, $x20, $x21, $x22, $x23, $x24, $x25, $x26, $x27, $x28, $d8, $d9, $d10, $d11, $d12, $d13, $d14, $x0
 
  $x1 = MOVZXi 0, 0
  TCRETURNdi @needx0, 0, implicit $sp

Yes, we should do is to check whether implict-def and implict use of $x0 are in the same instruction or not.

llvm/test/CodeGen/AArch64/machine-outliner-side-effect-2.mir
19–20

I wanted to show that this has nothing to do with the dead attribute, which is unnecessary and has been removed.

19–20

Why do you need to change this test file? With this fix, $x0 should be marked as use.

DianQK edited the summary of this revision. (Show Details)Nov 13 2021, 4:08 AM
Harbormaster completed remote builds in B134075: Diff 387013.Nov 13 2021, 4:37 AM
jinlin added a comment.Nov 13 2021, 1:58 PM

I have got your point. Please still keep your changes in file machine-outliner-side-effect-2.mir. Please update the comment in that file to state that the case of implict-def and implict use of $x0 in the same instruction are handled correctly.

DianQK updated this revision to Diff 387052.Nov 13 2021, 5:53 PM
DianQK marked an inline comment as done.

update machine-outliner-side-effect-2.mir

jinlin added a comment.Nov 13 2021, 6:14 PM
This comment was removed by jinlin.
jinlin accepted this revision.Nov 13 2021, 7:04 PM
This revision is now accepted and ready to land.Nov 13 2021, 7:04 PM
jinlin retitled this revision from Fix the side effect of outlined function when register is implicit and implicit-def to Fix the side effect of outlined function when register is implicit use and implicit-def in the same instruction..Nov 13 2021, 7:05 PM
jinlin retitled this revision from Fix the side effect of outlined function when register is implicit use and implicit-def in the same instruction. to Fix the side effect of outlined function when the register is implicit use and implicit-def in the same instruction..
Harbormaster completed remote builds in B134107: Diff 387052.Nov 13 2021, 7:15 PM
DianQK added a comment.Nov 13 2021, 9:01 PM

Many thanks for your review @jinlin. I don’t have commit access, can you land this patch for me? Please use “DianQK dianqk@icloud.com” to commit the change.

DianQK updated this revision to Diff 387063.Nov 14 2021, 3:22 AM

rebase the patch to make sure everything is based on an up-to-date origin/main.

Harbormaster completed remote builds in B134116: Diff 387063.Nov 14 2021, 3:54 AM
Closed by commit rG1e9fa0b12a76: Fix the side effect of outlined function when the register is implicit use and… (authored by DianQK, committed by jinlin). · Explain WhyNov 17 2021, 9:44 AM
This revision was automatically updated to reflect the committed changes.
jinlin added a commit: rG1e9fa0b12a76: Fix the side effect of outlined function when the register is implicit use and….

Revision Contents

PathSize
llvm/
lib/
CodeGen/
5 lines
test/
CodeGen/
AArch64/
11 lines
2 lines

Diff 384041

llvm/lib/CodeGen/MachineOutliner.cpp

//===---- MachineOutliner.cpp - Outline instructions -----------*- C++ -*-===// //===---- MachineOutliner.cpp - Outline instructions -----------*- C++ -*-===//
Lint: Lint
Inline Actions

clang-format not found in user’s local PATH; not linting file.

Lint: Lint: clang-format not found in user’s local PATH; not linting file.
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
/// ///
▲ Show 20 Lines • Show All 784 Lines • ▼ Show 20 Lines for (Candidate &C : OF.Candidates) {
// implicit Last inst in outlined range <-- def to the call // implicit Last inst in outlined range <-- def to the call
// instruction. Also remove call site information for outlined block // instruction. Also remove call site information for outlined block
// of code. The exposed uses need to be copied in the outlined range. // of code. The exposed uses need to be copied in the outlined range.
for (MachineBasicBlock::reverse_iterator for (MachineBasicBlock::reverse_iterator
Iter = EndIt.getReverse(), Iter = EndIt.getReverse(),
Last = std::next(CallInst.getReverse()); Last = std::next(CallInst.getReverse());
Iter != Last; Iter++) { Iter != Last; Iter++) {
MachineInstr *MI = &*Iter; MachineInstr *MI = &*Iter;
SmallSet<Register, 2> InstrUseRegs;
for (MachineOperand &MOP : MI->operands()) { for (MachineOperand &MOP : MI->operands()) {
// Skip over anything that isn't a register. // Skip over anything that isn't a register.
if (!MOP.isReg()) if (!MOP.isReg())
continue; continue;
if (MOP.isDef()) { if (MOP.isDef()) {
// Introduce DefRegs set to skip the redundant register. // Introduce DefRegs set to skip the redundant register.
DefRegs.insert(MOP.getReg()); DefRegs.insert(MOP.getReg());
if (!MOP.isDead() && UseRegs.count(MOP.getReg())) if (UseRegs.count(MOP.getReg()) &&
!InstrUseRegs.count(MOP.getReg()))
// Since the regiester is modeled as defined, // Since the regiester is modeled as defined,
// it is not necessary to be put in use register set. // it is not necessary to be put in use register set.
UseRegs.erase(MOP.getReg()); UseRegs.erase(MOP.getReg());
} else if (!MOP.isUndef()) { } else if (!MOP.isUndef()) {
// Any register which is not undefined should // Any register which is not undefined should
// be put in the use register set. // be put in the use register set.
UseRegs.insert(MOP.getReg()); UseRegs.insert(MOP.getReg());
InstrUseRegs.insert(MOP.getReg());
} }
} }
if (MI->isCandidateForCallSiteEntry()) if (MI->isCandidateForCallSiteEntry())
MI->getMF()->eraseCallSiteInfo(MI); MI->getMF()->eraseCallSiteInfo(MI);
} }
for (const Register &I : DefRegs) for (const Register &I : DefRegs)
// If it's a def, add it to the call instruction. // If it's a def, add it to the call instruction.
▲ Show 20 Lines • Show All 242 Lines • Show Last 20 Lines

llvm/test/CodeGen/AArch64/machine-outliner-side-effect-2.mir

Show All 10 Lines
--- ---
name: bax name: bax
tracksRegLiveness: true tracksRegLiveness: true
body: | body: |
bb.0: bb.0:
liveins: $x0, $lr liveins: $x0, $lr
$x1 = ADDXri $sp, 16, 0 $x1 = ADDXri $sp, 16, 0
BL @spam, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def dead $x0 BL @spam, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def $x0
renamable $x1 = COPY $x0
jinlinUnsubmitted
Done
ReplyInline Actions

Why do you need to change this test file? With this fix, $x0 should be marked as use.

jinlin: Why do you need to change this test file? With this fix, $x0 should be marked as use.
DianQKAuthorUnsubmitted
Done
ReplyInline Actions

I wanted to show that this has nothing to do with the dead attribute, which is unnecessary and has been removed.

DianQK: I wanted to show that this has nothing to do with the dead attribute, which is unnecessary and…
DianQKAuthorUnsubmitted
Done
ReplyInline Actions

Why do you need to change this test file? With this fix, $x0 should be marked as use.

DianQK: > Why do you need to change this test file? With this fix, $x0 should be marked as use.
RET_ReallyLR RET_ReallyLR
... ...
--- ---
name: bay name: bay
tracksRegLiveness: true tracksRegLiveness: true
body: | body: |
bb.0: bb.0:
liveins: $x0, $lr liveins: $x0, $lr
$x1 = ADDXri $sp, 16, 0 $x1 = ADDXri $sp, 16, 0
BL @spam, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def dead $x0 BL @spam, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def $x0
renamable $x2 = COPY $x0
RET_ReallyLR RET_ReallyLR
... ...
--- ---
name: baz name: baz
tracksRegLiveness: true tracksRegLiveness: true
body: | body: |
bb.0: bb.0:
liveins: $x0, $lr liveins: $x0, $lr
$x1 = ADDXri $sp, 16, 0 $x1 = ADDXri $sp, 16, 0
BL @spam, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def dead $x0 BL @spam, csr_darwin_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit killed $x1, implicit-def $sp, implicit-def $x0
renamable $x3 = COPY $x0
RET_ReallyLR RET_ReallyLR
... ...
# CHECK: BL @OUTLINED_FUNCTION_0, {{.*}}, implicit $x0 # CHECK: BL @OUTLINED_FUNCTION_0, {{.*}}, implicit $x0{{.*}}

llvm/test/CodeGen/AArch64/machine-outliner-side-effect.mir

Show All 23 Lines bb.0:
$x0 = COPY killed renamable $x20 $x0 = COPY killed renamable $x20
BL @spam, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit-def $sp, implicit-def $x0 BL @spam, csr_aarch64_aapcs, implicit-def dead $lr, implicit $sp, implicit $x0, implicit-def $sp, implicit-def $x0
renamable $x3 = COPY $x0 renamable $x3 = COPY $x0
RET_ReallyLR RET_ReallyLR
... ...
# CHECK: BL @OUTLINED_FUNCTION_0, {{.*}}, implicit $x20, {{.*}} # CHECK: BL @OUTLINED_FUNCTION_0, {{.*}}, implicit $x20{{.*}}