[X86] Use LivePhysRegs in X86FixupBWInsts.

[ab]

Summary

Kill-flags, which computeRegisterLiveness uses, are not reliable. LivePhysRegs is.

[qcolombet]

LGTM.

[kbsmith1]

LGTM

lib/Target/X86/X86FixupBWInsts.cpp

 /// targeted, or when optimizing for minimum code size.
 ///
 //===----------------------------------------------------------------------===//
 
 #include "X86.h"
 #include "X86InstrInfo.h"
 #include "X86Subtarget.h"
 #include "llvm/ADT/Statistic.h"
-#include "llvm/CodeGen/LiveVariables.h"
+#include "llvm/CodeGen/LivePhysRegs.h"
 #include "llvm/CodeGen/MachineFunctionPass.h"
 #include "llvm/CodeGen/MachineInstrBuilder.h"
 #include "llvm/CodeGen/MachineLoopInfo.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Target/TargetInstrInfo.h"
class FixupBWInstPass : public MachineFunctionPass {
 
   const char *getPassName() const override {
     return "X86 Byte/Word Instruction Fixup";
   }
 
   /// \brief Loop over all of the instructions in the basic block
   /// replacing applicable byte or word instructions with better
   /// alternatives.
-  void processBasicBlock(MachineFunction &MF, MachineBasicBlock &MBB) const;
+  void processBasicBlock(MachineFunction &MF, MachineBasicBlock &MBB);
 
   /// \brief This sets the \p SuperDestReg to the 32 bit super reg
   /// of the original destination register of the MachineInstr
   /// passed in. It returns true if that super register is dead
   /// just prior to \p OrigMI, and false if not.
   /// \pre OrigDestSize must be 8 or 16.
   bool getSuperRegDestIfDead(MachineInstr *OrigMI, unsigned OrigDestSize,
                              unsigned &SuperDestReg) const;
private:
   /// Machine instruction info used throughout the class.
   const X86InstrInfo *TII;
 
   /// Local member for function's OptForSize attribute.
   bool OptForSize;
 
   /// Machine loop info used for guiding some heruistics.
   MachineLoopInfo *MLI;
+
+  /// Register Liveness information after the current instruction.
+  LivePhysRegs LiveRegs;
 };
 char FixupBWInstPass::ID = 0;
 }
 
 FunctionPass *llvm::createX86FixupBWInsts() { return new FixupBWInstPass(); }
 
 bool FixupBWInstPass::runOnMachineFunction(MachineFunction &MF) {
   if (!FixupBWInsts)
     return false;
 
   this->MF = &MF;
   TII = MF.getSubtarget<X86Subtarget>().getInstrInfo();
   OptForSize = MF.getFunction()->optForSize();
   MLI = &getAnalysis<MachineLoopInfo>();
+  LiveRegs.init(&TII->getRegisterInfo());
 
   DEBUG(dbgs() << "Start X86FixupBWInsts\n";);
 
   // Process all basic blocks.
   for (auto &MBB : MF)
     processBasicBlock(MF, MBB);
 
   DEBUG(dbgs() << "End X86FixupBWInsts\n";);
bool FixupBWInstPass::getSuperRegDestIfDead(MachineInstr *OrigMI,
   // super-register is considered dead.
   // This test works because getX86SubSuperRegister returns the low portion
   // register by default when getting a sub-register, so if that doesn't
   // match the original destination register, then the original destination
   // register must not have been the low register portion of that size.
   if (getX86SubSuperRegister(SuperDestReg, OrigDestSize) != OrigDestReg)
     return false;
 
-  MachineBasicBlock::LivenessQueryResult LQR =
-      OrigMI->getParent()->computeRegisterLiveness(&TII->getRegisterInfo(),
-                                                   SuperDestReg, OrigMI);
-
-  if (LQR != MachineBasicBlock::LQR_Dead)
+  if (LiveRegs.contains(SuperDestReg))
     return false;
 
   if (OrigDestSize == 8) {
     // In the case of byte registers, we also have to check that the upper
     // byte register is also dead. That is considered to be independent of
     // whether the super-register is dead.
     unsigned UpperByteReg = getX86SubSuperRegister(SuperDestReg, 8, true);
 
-    LQR = OrigMI->getParent()->computeRegisterLiveness(&TII->getRegisterInfo(),
-                                                       UpperByteReg, OrigMI);
-    if (LQR != MachineBasicBlock::LQR_Dead)
+    if (LiveRegs.contains(UpperByteReg))
       return false;
   }
 
   return true;
 }
 
 MachineInstr *FixupBWInstPass::tryReplaceLoad(unsigned New32BitOpcode,
                                               unsigned OrigDestSize,
for (unsigned i = 1; i < NumArgs; ++i)
     MIB.addOperand(MI->getOperand(i));
 
   MIB->setMemRefs(MI->memoperands_begin(), MI->memoperands_end());
 
   return MIB;
 }
 
 void FixupBWInstPass::processBasicBlock(MachineFunction &MF,
-                                        MachineBasicBlock &MBB) const {
+                                        MachineBasicBlock &MBB) {
 
   // This algorithm doesn't delete the instructions it is replacing
   // right away.  By leaving the existing instructions in place, the
   // register liveness information doesn't change, and this makes the
   // analysis that goes on be better than if the replaced instructions
   // were immediately removed.
   //
   // This algorithm always creates a replacement instruction
   // and notes that and the original in a data structure, until the
   // whole BB has been analyzed.  This keeps the replacement instructions
   // from making it seem as if the larger register might be live.
   SmallVector<std::pair<MachineInstr *, MachineInstr *>, 8> MIReplacements;
 
-  for (MachineBasicBlock::iterator I = MBB.begin(); I != MBB.end(); ++I) {
+  // Start computing liveness for this block. We iterate from the end to be able
+  // to update this for each instruction.
+  LiveRegs.clear();
+  LiveRegs.addLiveOuts(&MBB);
+
+  for (auto I = MBB.rbegin(); I != MBB.rend(); ++I) {
     MachineInstr *NewMI = nullptr;
-    MachineInstr *MI = I;
+    MachineInstr *MI = &*I;
 
     // See if this is an instruction of the type we are currently looking for.
     switch (MI->getOpcode()) {
 
     case X86::MOV8rm:
       // Only replace 8 bit loads with the zero extending versions if
       // in an inner most loop and not optimizing for size. This takes
       // an extra byte to encode, and provides limited performance upside.
for (auto I = MBB.rbegin(); I != MBB.rend(); ++I) {
 
     default:
       // nothing to do here.
       break;
     }
 
     if (NewMI)
       MIReplacements.push_back(std::make_pair(MI, NewMI));
+
+    // We're done with this instruction, update liveness for the next one.
+    LiveRegs.stepBackward(*MI);
   }
 
   while (!MIReplacements.empty()) {
     MachineInstr *MI = MIReplacements.back().first;
     MachineInstr *NewMI = MIReplacements.back().second;
     MIReplacements.pop_back();
     MBB.insert(MI, NewMI);
     MBB.erase(MI);
   }
 }

test/CodeGen/X86/MergeConsecutiveStores.ll

 ; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+avx -fixup-byte-word-insts=1 < %s | FileCheck -check-prefix=CHECK -check-prefix=BWON %s
 ; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+avx -fixup-byte-word-insts=0 < %s | FileCheck -check-prefix=CHECK -check-prefix=BWOFF %s
-; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+avx -addr-sink-using-gep=1 < %s | FileCheck %s
+; RUN: llc -mtriple=x86_64-unknown-unknown -mattr=+avx -addr-sink-using-gep=1 < %s | FileCheck -check-prefix=CHECK -check-prefix=BWON %s
 
 %struct.A = type { i8, i8, i8, i8, i8, i8, i8, i8 }
 %struct.B = type { i32, i32, i32, i32, i32, i32, i32, i32 }
 
 ; CHECK-LABEL: merge_const_store:
 ; save 1,2,3 ... as one big integer.
 ; CHECK: movabsq $578437695752307201
 ; CHECK: ret
._crit_edge:                                      ; preds = %4, %0
   ret void
 }
 
 ; The loads and the stores are interleaved. Can't merge them.
 ; CHECK-LABEL: no_merge_loads:
 ; BWON:  movzbl
 ; BWOFF: movb
 ; CHECK: movb
-; CHECK: movb
+; BWON:  movzbl
+; BWOFF: movb
 ; CHECK: movb
 ; CHECK: ret
 define void @no_merge_loads(i32 %count, %struct.A* noalias nocapture %q, %struct.A* noalias nocapture %p) nounwind uwtable noinline ssp {
   %1 = icmp sgt i32 %count, 0
   br i1 %1, label %.lr.ph, label %._crit_edge
 
 .lr.ph:                                           ; preds = %0
   %2 = getelementptr inbounds %struct.A, %struct.A* %q, i64 0, i32 0

 
 ._crit_edge:                                      ; preds = %4, %0
   ret void
 }
 
 ; Make sure that we merge the consecutive load/store sequence below and use a
 ; word (16 bit) instead of a byte copy.
 ; CHECK-LABEL: MergeLoadStoreBaseIndexOffset:
-; CHECK: movw    (%{{.*}},%{{.*}}), [[REG:%[a-z]+]]
-; CHECK: movw    [[REG]], (%{{.*}})
+; BWON: movzwl   (%{{.*}},%{{.*}}), %e[[REG:[a-z]+]]
+; BWOFF: movw    (%{{.*}},%{{.*}}), %[[REG:[a-z]+]]
+; CHECK: movw    %[[REG]], (%{{.*}})
 define void @MergeLoadStoreBaseIndexOffset(i64* %a, i8* %b, i8* %c, i32 %n) {
   br label %1
 
 ; <label>:1
   %.09 = phi i32 [ %n, %0 ], [ %11, %1 ]
   %.08 = phi i8* [ %b, %0 ], [ %10, %1 ]
   %.0 = phi i64* [ %a, %0 ], [ %2, %1 ]
   %2 = getelementptr inbounds i64, i64* %.0, i64 1

 ; <label>:13
   ret void
 }
 
 ; Make sure that we merge the consecutive load/store sequence below and use a
 ; word (16 bit) instead of a byte copy even if there are intermediate sign
 ; extensions.
 ; CHECK-LABEL: MergeLoadStoreBaseIndexOffsetSext:
-; CHECK: movw    (%{{.*}},%{{.*}}), [[REG:%[a-z]+]]
-; CHECK: movw    [[REG]], (%{{.*}})
+; BWON: movzwl   (%{{.*}},%{{.*}}), %e[[REG:[a-z]+]]
+; BWOFF: movw    (%{{.*}},%{{.*}}), %[[REG:[a-z]+]]
+; CHECK: movw    %[[REG]], (%{{.*}})
 define void @MergeLoadStoreBaseIndexOffsetSext(i8* %a, i8* %b, i8* %c, i32 %n) {
   br label %1
 
 ; <label>:1
   %.09 = phi i32 [ %n, %0 ], [ %12, %1 ]
   %.08 = phi i8* [ %b, %0 ], [ %11, %1 ]
   %.0 = phi i8* [ %a, %0 ], [ %2, %1 ]
   %2 = getelementptr inbounds i8, i8* %.0, i64 1

test/CodeGen/X86/fixup-bw-inst.ll

 ; RUN: llc -fixup-byte-word-insts=1 -march=x86-64 < %s | \
 ; RUN: FileCheck -check-prefix CHECK -check-prefix BWON %s
 ; RUN: llc -fixup-byte-word-insts=0 -march=x86-64 < %s | \
 ; RUN: FileCheck -check-prefix CHECK -check-prefix BWOFF %s
 
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.8.0"
 
 %struct.A = type { i8, i8, i8, i8, i8, i8, i8, i8 }
 
 ; This has byte loads interspersed with byte stores, in a single
 ; basic-block loop.  The upper portion should be dead, so the movb loads
 ; should have been changed into movzbl instead.
-; TODO: The second movb load doesn't get fixed due to register liveness
-; not being accurate enough.
 ; CHECK-LABEL: foo1
 ; load:
 ; BWON:  movzbl
 ; BWOFF: movb
 ; store:
 ; CHECK: movb
 ; load:
-; CHECK: movb
+; BWON: movzbl
+; BWOFF: movb
 ; store:
 ; CHECK: movb
 ; CHECK: ret
 define void @foo1(i32 %count,
                   %struct.A* noalias nocapture %q,
                   %struct.A* noalias nocapture %p)
                     nounwind uwtable noinline ssp {
   %1 = icmp sgt i32 %count, 0
._crit_edge:                                      ; preds = %4, %0
   ret void
 }
 
 %struct.B = type { i16, i16, i16, i16, i16, i16, i16, i16 }
 
 ; This has word loads interspersed with word stores.
 ; The upper portion should be dead, so the movw loads should have
 ; been changed into movzwl instead.
-; TODO: The second movw load doesn't get fixed due to register liveness
-; not being accurate enough.
 ; CHECK-LABEL: foo2
 ; load:
 ; BWON:  movzwl
 ; BWOFF: movw
 ; store:
 ; CHECK: movw
 ; load:
-; CHECK: movw
+; BWON:  movzwl
+; BWOFF: movw
 ; store:
 ; CHECK: movw
 ; CHECK: ret
 define void @foo2(i32 %count,
                   %struct.B* noalias nocapture %q,
                   %struct.B* noalias nocapture %p)
                     nounwind uwtable noinline ssp {
   %1 = icmp sgt i32 %count, 0

test/CodeGen/X86/return-ext.ll

 ; RUN: llc < %s -mtriple=i686-unknown-linux-gnu -fixup-byte-word-insts=0 | \
 ; RUN:   FileCheck -check-prefix=CHECK -check-prefix=BWOFF %s
 ; RUN: llc < %s -mtriple=i686-unknown-linux-gnu -fixup-byte-word-insts=1 | \
 ; RUN:   FileCheck -check-prefix=CHECK -check-prefix=BWON %s
 ; RUN: llc < %s -mtriple=x86_64-unknown-linux-gnu -fixup-byte-word-insts=0 | \
 ; RUN:   FileCheck -check-prefix=CHECK -check-prefix=BWOFF %s
 ; RUN: llc < %s -mtriple=i686-unknown-linux-gnu -fixup-byte-word-insts=1 | \
 ; RUN:   FileCheck -check-prefix=CHECK -check-prefix=BWON %s
-; RUN: llc < %s -mtriple=x86_64-apple-darwin | FileCheck -check-prefix=DARWIN %s
+; RUN: llc < %s -mtriple=x86_64-apple-darwin -fixup-byte-word-insts=0 | \
+; RUN:   FileCheck -check-prefix=DARWIN -check-prefix=DARWIN-BWOFF %s
+; RUN: llc < %s -mtriple=x86_64-apple-darwin -fixup-byte-word-insts=1 | \
+; RUN:   FileCheck -check-prefix=DARWIN -check-prefix=DARWIN-BWON %s
 
 
 @x = common global i32 0, align 4
 
 define zeroext i1 @unsigned_i1() {
 entry:
   %0 = load i32, i32* @x
   %cmp = icmp eq i32 %0, 42

 ; CHECK-LABEL: unsigned_i16:
 ; BWOFF:       movw
 ; BWON:        movzwl
 ; CHECK-NEXT:  addw
 ; CHECK-NEXT:  ret
 
 ; Except on Darwin, for legay reasons.
 ; DARWIN-LABEL: unsigned_i16:
-; DARWIN:       movw
+; DARWIN-BWOFF: movw
+; DARWIN-BWON:  movzwl
 ; DARWIN-NEXT:  addw
 ; DARWIN-NEXT:  movzwl
 ; DARWIN-NEXT:  ret
 }
 
 
 define i32 @use_i1() {
 entry: