Page MenuHomePhabricator

MachineSink: permit sinking into INLINEASM_BR indirect targets
AbandonedPublic

Authored by nickdesaulniers on Jul 9 2020, 5:15 PM.

Details

Reviewers
jyknight
void
Summary

Fixes a kernel panic for 4.4 LTS x86_64 Linux kernels.

Fixes: D79794
Link: https://github.com/ClangBuiltLinux/linux/issues/1085
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>

Diff Detail

Unit TestsFailed

TimeTest
90 mswindows > LLVM.CodeGen/X86::Unknown Unit Message ("")
Script: -- : 'RUN: at line 1'; c:\ws\w-no-ssd\llvm-project\premerge-checks\build\bin\llc.exe -O2 $1 -print-after=machine-sink -stop-after=machine-sink -o /dev/null 2>&1 C:\ws\w-no-ssd\llvm-project\premerge-checks\llvm\test\CodeGen\X86\machine-sink-inlineasm-br.ll | c:\ws\w-no-ssd\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w-no-ssd\llvm-project\premerge-checks\llvm\test\CodeGen\X86\machine-sink-inlineasm-br.ll

Event Timeline

nickdesaulniers created this revision.Jul 9 2020, 5:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 9 2020, 5:15 PM

From the comment, there's obviously more we want to do here. Posting what I have for today since it's EOD. Will iterate on feedback tomorrow.

void added a comment.Jul 9 2020, 6:07 PM

I'm confused by this change. The original code *should* be correct. That it's resulting in this issue is another thing.

My first question is, why are we trying to sink instructions into the entry block?

void added a comment.Jul 10 2020, 2:33 PM

Without this change, the "ADD64ri8" instruction is before the INLINEASM_BR before machine sinking.

It looks like the issue shown in this test-case appears in Two-Address instruction pass, not Machine Sink.

We go from:

bb.0 (%ir-block.1):
  successors: %bb.2(0x80000000), %bb.1(0x00000000); %bb.2(100.00%), %bb.1(0.00%)
  liveins: $rdi
  %1:gr64 = COPY killed $rdi
  %0:gr64 = nuw ADD64ri8 %1:gr64(tied-def 0), 24, implicit-def dead $eflags
  INLINEASM_BR &"# $0 $1 $2" [sideeffect] [mayload] [maystore] [attdialect], $0:[mem:m], killed %1:gr64, 1, $noreg, 24, $noreg, $1:[imm], 1, $2:[imm], blockaddress(@klist_dec_and_del, %ir-block.4), $
3:[clobber], implicit-def dead early-clobber $df, $4:[clobber], implicit-def early-clobber $fpsw, $5:[clobber], implicit-def dead early-clobber $eflags
  JMP_1 %bb.2

bb.1 (%ir-block.4, address-taken):
; predecessors: %bb.0
  successors: %bb.2(0x80000000); %bb.2(100.00%)

  MOV64mi32 killed %0:gr64, 1, $noreg, -24, $noreg, 0 :: (store 8 into %ir.6)

bb.2 (%ir-block.7):
; predecessors: %bb.0, %bb.1

  RET 0, undef $eax

then replace the ADD64ri8 with a LEA64r -- but place it _after_ the INLINEASM_BR,

 bb.0 (%ir-block.1):
   successors: %bb.2(0x80000000), %bb.1(0x00000000); %bb.2(100.00%), %bb.1(0.00%)
   liveins: $rdi
   %1:gr64 = COPY killed $rdi
-  %0:gr64 = nuw ADD64ri8 %1:gr64(tied-def 0), 24, implicit-def dead $eflags
-  INLINEASM_BR &"# $0 $1 $2" [sideeffect] [mayload] [maystore] [attdialect], $0:[mem:m], killed %1:gr64, 1, $noreg, 24, $noreg, $1:[imm], 1, $2:[imm], blockaddress(@klist_dec_and_del, %ir-block.4), $3:[clobber], implicit-def dead early-clobber $df, $4:[clobber], implicit-def early-clobber $fpsw, $5:[clobber], implicit-def dead early-clobber $eflags
+  INLINEASM_BR &"# $0 $1 $2" [sideeffect] [mayload] [maystore] [attdialect], $0:[mem:m], %1:gr64, 1, $noreg, 24, $noreg, $1:[imm], 1, $2:[imm], blockaddress(@klist_dec_and_del, %ir-block.4), $3:[clobber], implicit-def dead early-clobber $df, $4:[clobber], implicit-def early-clobber $fpsw, $5:[clobber], implicit-def dead early-clobber $eflags
+  %0:gr64 = LEA64r killed %1:gr64, 1, $noreg, 24, $noreg
   JMP_1 %bb.2
nickdesaulniers planned changes to this revision.Jul 10 2020, 3:17 PM
void added a comment.Jul 12 2020, 7:54 PM

I think this might be a better fix:

diff --git a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
index de336abe607..d21407a60eb 100644
--- a/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
+++ b/llvm/lib/CodeGen/TwoAddressInstructionPass.cpp
@@ -888,6 +888,7 @@ rescheduleMIBelowKill(MachineBasicBlock::iterator &mi,
     return false;
 
   if (KillMI->hasUnmodeledSideEffects() || KillMI->isCall() ||
+      KillMI->getOpcode() == TargetOpcode::INLINEASM_BR ||
       KillMI->isBranch() || KillMI->isTerminator())
     // Don't move pass calls, etc.
     return false;
@@ -948,6 +949,7 @@ rescheduleMIBelowKill(MachineBasicBlock::iterator &mi,
       return false;
     ++NumVisited;
     if (OtherMI.hasUnmodeledSideEffects() || OtherMI.isCall() ||
+        OtherMI.getOpcode() == TargetOpcode::INLINEASM_BR ||
         OtherMI.isBranch() || OtherMI.isTerminator())
       // Don't move pass calls, etc.
       return false;
@@ -1122,6 +1124,7 @@ rescheduleKillAboveMI(MachineBasicBlock::iterator &mi,
       return false;
     ++NumVisited;
     if (OtherMI.hasUnmodeledSideEffects() || OtherMI.isCall() ||
+        OtherMI.getOpcode() == TargetOpcode::INLINEASM_BR ||
         OtherMI.isBranch() || OtherMI.isTerminator())
       // Don't move pass calls, etc.
       return false;
void added a comment.Jul 12 2020, 8:03 PM

Here are a few other places to contemplate similar changes:

diff --git a/llvm/lib/CodeGen/MachineCSE.cpp b/llvm/lib/CodeGen/MachineCSE.cpp
index 09531276bc1..a6873c7acba 100644
--- a/llvm/lib/CodeGen/MachineCSE.cpp
+++ b/llvm/lib/CodeGen/MachineCSE.cpp
@@ -404,6 +404,7 @@ bool MachineCSE::isCSECandidate(MachineInstr *MI) {
 
   // Ignore stuff that we obviously can't move.
   if (MI->mayStore() || MI->isCall() || MI->isTerminator() ||
+      MI->getOpcode() == TargetOpcode::INLINEASM_BR ||
       MI->mayRaiseFPException() || MI->hasUnmodeledSideEffects())
     return false;
 
diff --git a/llvm/lib/CodeGen/ReachingDefAnalysis.cpp b/llvm/lib/CodeGen/ReachingDefAnalysis.cpp
index 5bd8b4b8e27..90e829c925e 100644
--- a/llvm/lib/CodeGen/ReachingDefAnalysis.cpp
+++ b/llvm/lib/CodeGen/ReachingDefAnalysis.cpp
@@ -519,6 +519,7 @@ MachineInstr* ReachingDefAnalysis::getLocalLiveOutMIDef(MachineBasicBlock *MBB,
 static bool mayHaveSideEffects(MachineInstr &MI) {
   return MI.mayLoadOrStore() || MI.mayRaiseFPException() ||
          MI.hasUnmodeledSideEffects() || MI.isTerminator() ||
+         MI.getOpcode() == TargetOpcode::INLINEASM_BR ||
          MI.isCall() || MI.isBarrier() || MI.isBranch() || MI.isReturn();
 }

In https://github.com/ClangBuiltLinux/linux/issues/1085#issuecomment-656347490 @void suggested trying to mark INLINEASM_BR MCInsts as IsBarrier. Based on the latest recommendations, I'm wondering if IsCall is more appropriate?

I think this might be a better fix:

It seems that none of those fix the provided test case. :-(

I think this might be a better fix:

It seems that none of those fix the provided test case. :-(

Ah, right, because the test case is still testing machinesink. Let me change it to twoaddressinstruction and retest.

INLINEASM_BR already returns true for hasUnmodeledSideEffects(), so the diff above doesn't change anything.

The bug is in TwoAddressInstructinoPass::sink3AddrInstruction, which doesn't query _ANY_ of these predicates. I've just uploaded https://reviews.llvm.org/D83708 to remove the function entirely.

  • change to twoaddressinstruction
nickdesaulniers abandoned this revision.Jul 13 2020, 12:38 PM

Abandoning in preference to D83708.