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

Unit TestsFailed

TimeTest
1,950 msx64 debian > AddressSanitizer-x86_64-linux-dynamic.TestCases::exitcode.cpp
2,270 msx64 debian > AddressSanitizer-x86_64-linux.TestCases::exitcode.cpp

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 ↗(On Diff #384669)

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 ↗(On Diff #384669)

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

19 ↗(On Diff #384669)

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/
4 lines
test/
CodeGen/
AArch64/
machine-outliner-side-effect-3.mir
machine-outliner-side-effect.mir
12 lines
2 lines

Diff 383739

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 (!MOP.isDead() && UseRegs.count(MOP.getReg()) && !InstrUseRegs.count(MOP.getReg()))
Lint: Pre-merge checks
Inline Actions

clang-format: please reformat the code

-              if (!MOP.isDead() && UseRegs.count(MOP.getReg()) && !InstrUseRegs.count(MOP.getReg()))
+              if (!MOP.isDead() && UseRegs.count(MOP.getReg()) &&
+                  !InstrUseRegs.count(MOP.getReg()))
Lint: Pre-merge checks: clang-format: please reformat the code ``` - if (!MOP.isDead() && UseRegs.count…
// 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-3.mir

  • This file was copied from llvm/test/CodeGen/AArch64/machine-outliner-side-effect.mir.
# RUN: llc -mtriple=aarch64 -run-pass=machine-outliner -verify-machineinstrs %s -o - | FileCheck %s # 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. # The test checks whether the compiler updates the side effect of function @OUTLINED_FUNCTION_0 when passing register x0.
--- | --- |
declare void @spam() local_unnamed_addr declare void @spam() local_unnamed_addr
define void @baz() optsize minsize noredzone { ret void } define void @baz() optsize minsize noredzone { ret void }
... ...
--- ---
name: baz name: baz
tracksRegLiveness: true tracksRegLiveness: true
body: | body: |
bb.0: bb.0:
liveins: $x0, $x20 liveins: $x0
$x0 = COPY renamable $x20 $w1 = MOVZWi 0, 0
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 $x21 = COPY $x0 renamable $x21 = COPY $x0
$x0 = COPY renamable $x20 $w1 = MOVZWi 0, 0
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 $x22 = COPY $x0 renamable $x22 = COPY $x0
$x0 = COPY killed renamable $x20 $w1 = MOVZWi 0, 0
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 $x0{{.*}}

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

  • This file was copied to llvm/test/CodeGen/AArch64/machine-outliner-side-effect-3.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{{.*}}