[PATCH] critical-anti-dependency breaker: don't use reg def info from kill insts (PR20308)

[Sanjay Patel]

Hi hfinkel, atrick,

In PR20308 ( http://llvm.org/bugs/show_bug.cgi?id=20308 ), the critical-anti-dependency breaker caused a miscompile because it broke a WAR hazard using a register that it thinks is available based on info from a kill inst. We should never use any def/use info from a kill because they are really just nops.

This patch adds guard checks for kills around calls to ScanInstruction() where the DefIndices array is set. For good measure, add an assert in ScanInstruction() so we don't hit this bug again.

The test case is a reduced version of the code from the bug report.

http://reviews.llvm.org/D4977

Files:
  lib/CodeGen/CriticalAntiDepBreaker.cpp
  test/CodeGen/X86/critical-anti-dep-breaker.ll

Index: lib/CodeGen/CriticalAntiDepBreaker.cpp
===================================================================
--- lib/CodeGen/CriticalAntiDepBreaker.cpp
+++ lib/CodeGen/CriticalAntiDepBreaker.cpp
@@ -91,7 +91,9 @@
 
 void CriticalAntiDepBreaker::Observe(MachineInstr *MI, unsigned Count,
                                      unsigned InsertPosIndex) {
-  if (MI->isDebugValue())
+  // Kill insts can have reg def information even though they are nops and
+  // will probably be removed. We must ignore them.
+  if (MI->isDebugValue() || MI->isKill())
     return;
   assert(Count < InsertPosIndex && "Instruction index out of expected range!");
 
@@ -234,6 +236,7 @@
   // Update liveness.
   // Proceeding upwards, registers that are defed but not used in this
   // instruction are now dead.
+  assert(!MI->isKill() && "Attempting to scan a kill instruction");
 
   if (!TII->isPredicated(MI)) {
     // Predicated defs are modeled as read + write, i.e. similar to two
@@ -505,7 +508,9 @@
   unsigned Count = InsertPosIndex - 1;
   for (MachineBasicBlock::iterator I = End, E = Begin; I != E; --Count) {
     MachineInstr *MI = --I;
-    if (MI->isDebugValue())
+    // Kill insts can have reg def information even though they are nops and
+    // will probably be removed. We must ignore them.
+    if (MI->isDebugValue() || MI->isKill())
       continue;
 
     // Check if this instruction has a dependence on the critical path that
Index: test/CodeGen/X86/critical-anti-dep-breaker.ll
===================================================================
--- test/CodeGen/X86/critical-anti-dep-breaker.ll
+++ test/CodeGen/X86/critical-anti-dep-breaker.ll
@@ -0,0 +1,28 @@
+; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -relocation-model=pic -post-RA-scheduler=1 -break-anti-dependencies=critical  | FileCheck %s
+
+; PR20308 ( http://llvm.org/bugs/show_bug.cgi?id=20308 )
+; The critical-anti-dependency-breaker must not use register def information from a kill inst.
+; This test case expects such an instruction to appear as a comment with def info for RDI.
+; There is an anti-dependency (WAR) hazard using RAX using default reg allocation and scheduling.
+; The post-RA-scheduler and critical-anti-dependency breaker can eliminate that hazard using R10.
+; That is the first free register that isn't used as a param in the call to "@Image".
+
+ at PartClass = external global i32
+ at NullToken = external global i64
+
+; CHECK-LABEL: Part_Create:
+; CHECK-DAG: # kill: RDI<def> 
+; CHECK-DAG: movq PartClass at GOTPCREL(%rip), %r10
+define i32 @Part_Create(i64* %Anchor, i32 %TypeNum, i32 %F, i32 %Z, i32* %Status, i64* %PartTkn) {
+  %PartObj = alloca i64*, align 8
+  %Vchunk = alloca i64, align 8
+  %1 = load i64* @NullToken, align 4
+  store i64 %1, i64* %Vchunk, align 8
+  %2 = load i32* @PartClass, align 4
+  call i32 @Image(i64* %Anchor, i32 %2, i32 0, i32 0, i32* %Status, i64* %PartTkn, i64** %PartObj)
+  call i32 @Create(i64* %Anchor)
+  ret i32 %2
+}
+
+declare i32 @Image(i64*, i32, i32, i32, i32*, i64*, i64**)
+declare i32 @Create(i64*)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D4977.12686.patch
Type: text/x-patch
Size: 3064 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140819/949bc9ef/attachment.bin>