CodeGen: Redo analyzePhysRegs() and computeRegisterLiveness()

[MatzeB]

Summary

computeRegisterLiveness() was broken in that it reported dead for a
register even if a subregister was alive. I assume this was because the
results of analayzePhysRegs() are hard to understand with respect to
subregisters.

This commit: Changes the results of analyzePhysRegs (=struct
PhysRegInfo) to be clearly understandable, also renames the fields to
avoid silent breakage of third-party code (and improve the grammar).

Fix all (two) users of computeRegisterLiveness() in llvm: By reenabling
it and removing workarounds for the bug.

This fixes http://llvm.org/PR24535 and http://llvm.org/PR25033

[sanjoy]

Minor nits inline.

include/llvm/CodeGen/MachineInstrBundle.h

169

Please add a one-liner about what 'clobbers' means.

lib/CodeGen/MachineInstrBundle.cpp

328

Nit: repeated ; s.

[MatzeB]

Fix reported nitpicks and some improve some small things I found myself (continue shortcut in a loop, comment formatting)

include/llvm/CodeGen/MachineInstrBundle.h

169

I hopefully made it more clear what this flag means and referenced MachineOperand::CreateRegMask() for a definition of what the bits in the regmask mean.

[jfb]

Thanks for looking into this!

Did you re-run the repro programs in the bugs linked here? I want to make sure they all still pass.

include/llvm/CodeGen/MachineInstrBundle.h

190

Can you change uses of "overlapping" here and elsewhere? I think they're part of the confusion.

lib/Target/X86/X86InstrInfo.cpp

4454

Could you also fix peephole-na-phys-copy-folding.ll? Or does it trigger the verifier as the new FIXME says? We could pass a bigger Neighborhood value to computeRegisterLiveness if that's the case.

[MatzeB]

include/llvm/CodeGen/MachineInstrBundle.h

190

Hmm, not sure what a better description would be.
What about "Reg or one of its aliases is defined. The definition may only cover parts of the register." and then
"Reg or a super-register is defined. The definition covers the full register." below?

lib/Target/X86/X86InstrInfo.cpp

4454

Yes peephole-na-phys-copy-folding is affected by the FIXME I added (I'll add the name of the test to the comment). This is still an unsolved issue before and after the changes. Increasing the Neighborhood does not help as we need an "infinite" neighborhood to cover all cases. To get a precise liveness query we would need a LivePhysRegs (or RegisterScavenger) instance which ideally is kept up to date while we walk backwards over the basic blocks unfortunately ExpandPostRAPseudos has no easy way to maintain state and walks forward at the moment, so fixing this properly is work for another patch.

[jfb]

include/llvm/CodeGen/MachineInstrBundle.h

190

Sub-register?

And / or ASCII art comment :-)

lib/Target/X86/X86InstrInfo.cpp

4454

OK, keeping the comment here would be nice. The test file also has a FIXME which you seem to say can't be fixed while addressing the issue it mentions? Maybe it would be worth pulling out the tests that fail validation into their own test that don't have validation (so the other ones can be validated independently)?

[hfinkel]

lib/Target/X86/X86InstrInfo.cpp

4446

This seems like an odd FIXME to keep now that we have a way to detect the situation. Why don't you detect the LQR_Unknown here and abort instead of doing something the verifier will complain about? Then we can have a FIXME that we really should be using some real analysis here instead of a local search.

[jfb]

lib/Target/X86/X86InstrInfo.cpp

4446

I'm not sure I understand what you're suggesting, maybe the new FIXME isn't clear enough: you're right that a real analysis would fix the issue, but if the current one says unknown then we have to save/restore AX otherwise we may be clobbering live state. It's better to complain at verification time rather than to clobber state!

[hfinkel]

lib/Target/X86/X86InstrInfo.cpp

4446

Indeed, thanks for clarifying. You're right, we can't do better than this for now.

[jfb]

lib/Target/X86/X86InstrInfo.cpp

4446

OK, it would be good to clarify the FIXME then, if Hal didn't get it then it's not clear enough :)

[MatzeB]

include/llvm/CodeGen/MachineInstrBundle.h

190

I meant that the 2nd quote goes to the 2nd variable like this:

/// Reg or one of its aliases is defined. The definition may only cover
/// parts of the register.
bool Defined;
/// Reg or a super-register is defined. The definition covers the full
/// register.
bool FullyDefined;

/// Reg or ont of its aliases is read. The register may only be read
/// partially.
bool Read;
/// Reg or a super-register is read. The full register is read.
bool FullyRead;

I added the 2nd sentence in all these comments so you do not necessarily have to understand what sub-/super-register means here. To me the comments are perfectly clear now. This is not the place to explain concepts like sub- and superregisters with ASCII art IMO (keep in mind that this is a rarely used heuristic for circumstances where you cannot get the RegisterScavenger/LivePhysRegs working).

lib/Target/X86/X86InstrInfo.cpp

4446

Again I am not sure where the confusion comes from (obviously I try to write comments that make sense to myself). I am guessing the thing here is the MI representation (or rather the MachineVerifier) requires you to add the "undef" flag when reading from dead registers. We do not (and should not IMO) have a way to express a situation in which we do not know whether a register is dead or not.

Anyway I'll change it to this:

// FIXME: If computeRegisterLiveness() reported LQR_Unknown then AX may
// actually be dead. This is not a problem for correctness as we are just
// (unnecessarily) saving+restoring a dead register. However the
// MachineVerifier expects operands that read from dead registers
// to be marked with the "undef" flag.

4454

Strange, I looked at peephole-na-phys-copy-folding again and it works with -verify-machineinstrs (there are not emergency push ax generated anymore). I must have tested an intermediate version. So I'll just remove the FIXME and add -verify-machineinstrs to peephole-na-phys-copy-folding.ll

[qcolombet]

Hi Matthias,

LGTM.

Thanks,
Q.

[jfb]

lgtm after the peephole-na-phys-copy-folding.ll update and the two proposed comment updates. Thanks!

include/llvm/CodeGen/MachineBasicBlock.h

public:
   /// instructions.  Return UnknownLoc if there is none.
   DebugLoc findDebugLoc(instr_iterator MBBI);
   DebugLoc findDebugLoc(iterator MBBI) {
     return findDebugLoc(MBBI.getInstrIterator());
   }
 
   /// Possible outcome of a register liveness query to computeRegisterLiveness()
   enum LivenessQueryResult {
-    LQR_Live,            ///< Register is known to be live.
-    LQR_OverlappingLive, ///< Register itself is not live, but some overlapping
-                         ///< register is.
-    LQR_Dead,            ///< Register is known to be dead.
-    LQR_Unknown          ///< Register liveness not decidable from local
-                         ///< neighborhood.
+    LQR_Live,   ///< Register is known to be (at least partially) live.
+    LQR_Dead,   ///< Register is known to be fully dead.
+    LQR_Unknown ///< Register liveness not decidable from local
   };
 
   /// Return whether (physical) register \p Reg has been <def>ined and not
   /// <kill>ed as of just before \p Before.
   ///
   /// Search is localised to a neighborhood of \p Neighborhood instructions
   /// before (searching for defs or kills) and \p Neighborhood instructions
   /// after (searching just for defs) \p Before.

include/llvm/CodeGen/MachineInstrBundle.h

struct VirtRegInfo {
     bool Writes;
 
     /// Tied - Uses and defs must use the same register. This can be because of
     /// a two-address constraint, or there may be a partial redefinition of a
     /// sub-register.
     bool Tied;
   };
 
-  /// PhysRegInfo - Information about a physical register used by a set of
-  /// operands.
+  /// Information about a physical register use by a set of operands.
   struct PhysRegInfo {
-    /// Clobbers - Reg or an overlapping register is defined, or a regmask
-    /// clobbers Reg.
-    bool Clobbers;
+    /// A regmask clobbers Reg.
+    bool Clobbered;
 
-    /// Defines - Reg or a super-register is defined.
-    bool Defines;
+    /// Reg or an overlapping register is defined.
+    bool Defined;
+    /// Reg or a super-register is defined.
+    bool FullyDefined;
+
+    /// Reg or an overlapping register is read.
+    bool Read;
+    /// Reg or a super-register is read.
+    bool FullyRead;
+
+    /// Register is FullyDefined and all defs of reg or an overlapping register
+    /// are dead.
+    bool DeadDef;
 
-    /// Reads - Reg or a super-register is read.
-    bool Reads;
-
-    /// ReadsOverlap - Reg or an overlapping register is read.
-    bool ReadsOverlap;
-
-    /// DefinesDead - All defs of a Reg or a super-register are dead.
-    bool DefinesDead;
-
-    /// There is a kill of Reg or a super-register.
-    bool Kills;
+    /// There is a use operand of Reg or a super-register with kill flag set.
+    bool Killed;
   };
 
   /// analyzeVirtReg - Analyze how the current instruction or bundle uses a
   /// virtual register.  This function should not be called after operator++(),
   /// it expects a fresh iterator.
   ///
   /// @param Reg The virtual register to analyze.
   /// @param Ops When set, this vector will receive an (MI, OpNum) entry for
   ///            each operand referring to Reg.
   /// @returns A filled-in RegInfo struct.
   VirtRegInfo analyzeVirtReg(unsigned Reg,

lib/CodeGen/InlineSpiller.cpp

if (!Reg || TargetRegisterInfo::isVirtualRegister(Reg) ||
         MRI.isReserved(Reg)) {
       continue;
     }
     // Skip non-Defs, including undef uses and internal reads.
     if (MO->isUse())
       continue;
     MIBundleOperands::PhysRegInfo RI =
       MIBundleOperands(FoldMI).analyzePhysReg(Reg, &TRI);
-    if (RI.Defines)
+    if (RI.Defined)
       continue;
     // FoldMI does not define this physreg. Remove the LI segment.
     assert(MO->isDead() && "Cannot fold physreg def");
     SlotIndex Idx = LIS.getInstructionIndex(MI).getRegSlot();
     LIS.removePhysRegDefAt(Reg, Idx);
   }
 
   LIS.ReplaceMachineInstrInMaps(MI, FoldMI);

lib/CodeGen/MachineBasicBlock.cpp

MachineBasicBlock::computeRegisterLiveness(const TargetRegisterInfo *TRI,
 
   // Start by searching backwards from Before, looking for kills, reads or defs.
   const_iterator I(Before);
   // If this is the first insn in the block, don't search backwards.
   if (I != begin()) {
     do {
       --I;
 
-      MachineOperandIteratorBase::PhysRegInfo Analysis =
+      MachineOperandIteratorBase::PhysRegInfo Info =
         ConstMIOperands(I).analyzePhysReg(Reg, TRI);
 
-      if (Analysis.Defines)
-        // Outputs happen after inputs so they take precedence if both are
-        // present.
-        return Analysis.DefinesDead ? LQR_Dead : LQR_Live;
 
-      if (Analysis.Kills || Analysis.Clobbers)
-        // Register killed, so isn't live.
+      // Register is dead after a dead def of the full register.
+      if (Info.DeadDef)
         return LQR_Dead;
-
-      else if (Analysis.ReadsOverlap)
-        // Defined or read without a previous kill - live.
-        return Analysis.Reads ? LQR_Live : LQR_OverlappingLive;
-
+      // Register is (at least partially) live after a def.
+      if (Info.Defined)
+        return LQR_Live;
+      // Register is dead after a full kill or clobber and no def.
+      if (Info.Killed || Info.Clobbered)
+        return LQR_Dead;
+      // Register must be live if we read it.
+      if (Info.Read)
+        return LQR_Live;
     } while (I != begin() && --N > 0);
   }
 
   // Did we get to the start of the block?
   if (I == begin()) {
     // If so, the register's state is definitely defined by the live-in state.
-    for (MCRegAliasIterator RAI(Reg, TRI, /*IncludeSelf=*/true);
-         RAI.isValid(); ++RAI) {
+    for (MCRegAliasIterator RAI(Reg, TRI, /*IncludeSelf=*/true); RAI.isValid();
+         ++RAI)
       if (isLiveIn(*RAI))
-        return (*RAI == Reg) ? LQR_Live : LQR_OverlappingLive;
-    }
+        return LQR_Live;
 
     return LQR_Dead;
   }
 
   N = Neighborhood;
 
   // Try searching forwards from Before, looking for reads or defs.
   I = const_iterator(Before);
   // If this is the last insn in the block, don't search forwards.
   if (I != end()) {
     for (++I; I != end() && N > 0; ++I, --N) {
-      MachineOperandIteratorBase::PhysRegInfo Analysis =
+      MachineOperandIteratorBase::PhysRegInfo Info =
         ConstMIOperands(I).analyzePhysReg(Reg, TRI);
 
-      if (Analysis.ReadsOverlap)
-        // Used, therefore must have been live.
-        return (Analysis.Reads) ?
-          LQR_Live : LQR_OverlappingLive;
-
-      else if (Analysis.Clobbers || Analysis.Defines)
-        // Defined (but not read) therefore cannot have been live.
+      // Register is live when we read it here.
+      if (Info.Read)
+        return LQR_Live;
+      // Register is dead if we can fully overwrite or clobber it here.
+      if (Info.FullyDefined || Info.Clobbered)
         return LQR_Dead;
     }
   }
 
   // At this point we have no idea of the liveness of the register.
   return LQR_Unknown;
 }
 

lib/CodeGen/MachineInstrBundle.cpp

MachineOperandIteratorBase::analyzeVirtReg(unsigned Reg,
   }
   return RI;
 }
 
 MachineOperandIteratorBase::PhysRegInfo
 MachineOperandIteratorBase::analyzePhysReg(unsigned Reg,
                                            const TargetRegisterInfo *TRI) {
   bool AllDefsDead = true;
-  PhysRegInfo PRI = {false, false, false, false, false, false};
+  PhysRegInfo PRI = {false, false, false, false, false, false, false};
 
   assert(TargetRegisterInfo::isPhysicalRegister(Reg) &&
          "analyzePhysReg not given a physical register!");
   for (; isValid(); ++*this) {
     MachineOperand &MO = deref();
 
     if (MO.isRegMask() && MO.clobbersPhysReg(Reg))
-      PRI.Clobbers = true;    // Regmask clobbers Reg.
+      PRI.Clobbered = true;
 
     if (!MO.isReg())
       continue;
 
     unsigned MOReg = MO.getReg();
     if (!MOReg || !TargetRegisterInfo::isPhysicalRegister(MOReg))
       continue;
 
-    bool IsRegOrSuperReg = MOReg == Reg || TRI->isSuperRegister(MOReg, Reg);
-    bool IsRegOrOverlapping = MOReg == Reg || TRI->regsOverlap(MOReg, Reg);
-
-    if (IsRegOrSuperReg && MO.readsReg()) {
-      // Reg or a super-reg is read, and perhaps killed also.
-      PRI.Reads = true;
-      PRI.Kills = MO.isKill();
-    }
-
-    if (IsRegOrOverlapping && MO.readsReg()) {
-      PRI.ReadsOverlap = true;// Reg or an overlapping register is read.
-    }
-
-    if (!MO.isDef())
+    bool Overlapping = TRI->regsOverlap(MOReg, Reg);
+    if (!Overlapping)
       continue;
 
-    if (IsRegOrSuperReg) {
-      PRI.Defines = true;     // Reg or a super-register is defined.
+    bool Covered = TRI->isSuperRegisterEq(MOReg, Reg);
+    if (MO.readsReg()) {
+      PRI.Read = true;
+      if (Covered) {
+        PRI.FullyRead = true;
+        if (MO.isKill())
+          PRI.Killed = true;
+      }
+    } else if (MO.isDef()) {
+      PRI.Defined = true;
+      if (Covered)
+        PRI.FullyDefined = true;;
       if (!MO.isDead())
         AllDefsDead = false;
     }
-    if (IsRegOrOverlapping)
-      PRI.Clobbers = true;    // Reg or an overlapping reg is defined.
   }
 
-  if (AllDefsDead && PRI.Defines)
-    PRI.DefinesDead = true;   // Reg or super-register was defined and was dead.
+  if (AllDefsDead && PRI.FullyDefined)
+    PRI.DeadDef = true;
 
   return PRI;
 }

lib/Target/AArch64/AArch64ConditionalCompares.cpp

for (MachineBasicBlock::iterator B = MBB->begin(); I != B;) {
     case AArch64::FCMPEDrr:
       return I;
     }
 
     // Check for flag reads and clobbers.
     MIOperands::PhysRegInfo PRI =
         MIOperands(I).analyzePhysReg(AArch64::NZCV, TRI);
 
-    if (PRI.Reads) {
+    if (PRI.Read) {
       // The ccmp doesn't produce exactly the same flags as the original
       // compare, so reject the transform if there are uses of the flags
       // besides the terminators.
       DEBUG(dbgs() << "Can't create ccmp with multiple uses: " << *I);
       ++NumMultNZCVUses;
       return nullptr;
     }
 
-    if (PRI.Clobbers) {
+    if (PRI.Defined || PRI.Clobbered) {
       DEBUG(dbgs() << "Not convertible compare: " << *I);
       ++NumUnknNZCVDefs;
       return nullptr;
     }
   }
   DEBUG(dbgs() << "Flags not defined in BB#" << MBB->getNumber() << '\n');
   return nullptr;
 }

lib/Target/ARM/ARMBaseInstrInfo.cpp

while (NumBytes) {
     BuildMI(MBB, MBBI, dl, TII.get(Opc), DestReg)
       .addReg(BaseReg, RegState::Kill).addImm(ThisVal)
       .addImm((unsigned)Pred).addReg(PredReg).addReg(0)
       .setMIFlags(MIFlags);
     BaseReg = DestReg;
   }
 }
 
-static bool isAnySubRegLive(unsigned Reg, const TargetRegisterInfo *TRI,
-                      MachineInstr *MI) {
-  for (MCSubRegIterator Subreg(Reg, TRI, /* IncludeSelf */ true);
-       Subreg.isValid(); ++Subreg)
-    if (MI->getParent()->computeRegisterLiveness(TRI, *Subreg, MI) !=
-        MachineBasicBlock::LQR_Dead)
-      return true;
-  return false;
-}
 bool llvm::tryFoldSPUpdateIntoPushPop(const ARMSubtarget &Subtarget,
                                       MachineFunction &MF, MachineInstr *MI,
                                       unsigned NumBytes) {
   // This optimisation potentially adds lots of load and store
   // micro-operations, it's only really a great benefit to code-size.
   if (!MF.getFunction()->optForMinSize())
     return false;
 
if (!IsPop) {
       --RegsNeeded;
       continue;
     }
 
     // However, we can only pop an extra register if it's not live. For
     // registers live within the function we might clobber a return value
     // register; the other way a register can be live here is if it's
     // callee-saved.
-    // TODO: Currently, computeRegisterLiveness() does not report "live" if a
-    // sub reg is live. When computeRegisterLiveness() works for sub reg, it
-    // can replace isAnySubRegLive().
     if (isCalleeSavedRegister(CurReg, CSRegs) ||
-        isAnySubRegLive(CurReg, TRI, MI)) {
+        MI->getParent()->computeRegisterLiveness(TRI, CurReg, MI) !=
+        MachineBasicBlock::LQR_Dead) {
       // VFP pops don't allow holes in the register list, so any skip is fatal
       // for our transformation. GPR pops do, so we should just keep looking.
       if (IsVFPPushPop)
         return false;
       else
         continue;
     }
 

lib/Target/X86/X86InstrInfo.cpp

if ((FromEFLAGS || ToEFLAGS) && (is32 || is64)) {
     //
     // PUSHF/POPF is also potentially incorrect because it affects other flags
     // such as TF/IF/DF, which LLVM doesn't model.
     //
     // Notice that we have to adjust the stack if we don't want to clobber the
     // first frame index. See X86FrameLowering.cpp - clobbersTheStack.
 
 
-    bool AXDead = (Reg == AX);
-    // FIXME: The above could figure out that AX is dead in more cases with:
-    //          || (MachineBasicBlock::LQR_Dead ==
-    //            MBB.computeRegisterLiveness(&getRegisterInfo(), AX, MI));
-    //
-    //        Unfortunately this is slightly broken, see PR24535 and the likely
-    //        related PR25033 PR24991 PR24992 PR25201. These issues seem to
-    //        showcase sub-register / super-register confusion: a previous kill
-    //        of AH but no kill of AL leads computeRegisterLiveness to
-    //        erroneously conclude that AX is dead.
-    //
-    //        Once fixed, also update cmpxchg-clobber-flags.ll and
-    //        peephole-na-phys-copy-folding.ll.
-
-    if (!AXDead)
+    bool AXDead = (Reg == AX) ||
+                  (MachineBasicBlock::LQR_Dead ==
+                   MBB.computeRegisterLiveness(&getRegisterInfo(), AX, MI));
+    if (!AXDead) {
+      // FIXME: If computeRegisterLiveness() reported LQR_Unknown and AX is
+      // dead even though we couldn't determine that, then we are reading an
+      // undefined value here and the machine verifier will complain.
       BuildMI(MBB, MI, DL, get(Push)).addReg(AX, getKillRegState(true));
+    }
     if (FromEFLAGS) {
       BuildMI(MBB, MI, DL, get(X86::SETOr), X86::AL);
       BuildMI(MBB, MI, DL, get(X86::LAHF));
       BuildMI(MBB, MI, DL, get(Mov), Reg).addReg(AX);
     }
     if (ToEFLAGS) {
       BuildMI(MBB, MI, DL, get(Mov), AX).addReg(Reg, getKillRegState(KillSrc));
       BuildMI(MBB, MI, DL, get(X86::ADD8ri), X86::AL)

test/CodeGen/X86/cmpxchg-clobber-flags.ll

-; RUN: llc -mtriple=i386-linux-gnu %s -o - | FileCheck %s -check-prefix=i386
-; RUN: llc -mtriple=i386-linux-gnu -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=i386f
+; RUN: llc -verify-machineinstrs -mtriple=i386-linux-gnu %s -o - | FileCheck %s -check-prefix=i386
+; RUN: llc -verify-machineinstrs -mtriple=i386-linux-gnu -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=i386f
 
-; RUN: llc -mtriple=x86_64-linux-gnu %s -o - | FileCheck %s -check-prefix=x8664
-; RUN: llc -mtriple=x86_64-linux-gnu -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=x8664
-; RUN: llc -mtriple=x86_64-linux-gnu -mattr=+sahf %s -o - | FileCheck %s -check-prefix=x8664-sahf
-; RUN: llc -mtriple=x86_64-linux-gnu -mattr=+sahf -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=x8664-sahf
-; RUN: llc -mtriple=x86_64-linux-gnu -mcpu=corei7 %s -o - | FileCheck %s -check-prefix=x8664-sahf
-
-; FIXME: X86InstrInfo::copyPhysReg had code which figured out whether AX was
-;        live or not to avoid save / restore when it's not needed. See FIXME in
-;        that function for more details on which the code is currently
-;        disabled. The extra push/pop are marked below and can be removed once
-;        the issue is fixed.
-;        -verify-machineinstrs should also be added back in the RUN lines above.
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-linux-gnu %s -o - | FileCheck %s -check-prefix=x8664
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-linux-gnu -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=x8664
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-linux-gnu -mattr=+sahf %s -o - | FileCheck %s -check-prefix=x8664-sahf
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-linux-gnu -mattr=+sahf -pre-RA-sched=fast %s -o - | FileCheck %s -check-prefix=x8664-sahf
+; RUN: llc -verify-machineinstrs -mtriple=x86_64-linux-gnu -mcpu=corei7 %s -o - | FileCheck %s -check-prefix=x8664-sahf
 
 declare i32 @foo()
 declare i32 @bar(i64)
 
 define i64 @test_intervening_call(i64* %foo, i64 %bar, i64 %baz) {
 ; i386-LABEL: test_intervening_call:
 ; i386: cmpxchg8b
 ; i386-NEXT: pushl %eax
 ; i386-NEXT: seto %al
 ; i386-NEXT: lahf
 ; i386-NEXT: movl %eax, [[FLAGS:%.*]]
 ; i386-NEXT: popl %eax
 ; i386-NEXT: movl %edx, 4(%esp)
 ; i386-NEXT: movl %eax, (%esp)
 ; i386-NEXT: calll bar
-; ** FIXME Next line isn't actually necessary. **
-; i386-NEXT: pushl %eax
 ; i386-NEXT: movl [[FLAGS]], %eax
 ; i386-NEXT: addb $127, %al
 ; i386-NEXT: sahf
-; ** FIXME Next line isn't actually necessary. **
-; i386-NEXT: popl %eax
 ; i386-NEXT: jne
 
 ; i386f-LABEL: test_intervening_call:
 ; i386f: cmpxchg8b
 ; i386f-NEXT: movl %eax, (%esp)
 ; i386f-NEXT: movl %edx, 4(%esp)
-; ** FIXME Next line isn't actually necessary. **
-; i386f-NEXT: pushl %eax
 ; i386f-NEXT: seto %al
 ; i386f-NEXT: lahf
 ; i386f-NEXT: movl %eax, [[FLAGS:%.*]]
-; ** FIXME Next line isn't actually necessary. **
-; i386f-NEXT: popl %eax
 ; i386f-NEXT: calll bar
-; ** FIXME Next line isn't actually necessary. **
-; i386f-NEXT: pushl %eax
 ; i386f-NEXT: movl [[FLAGS]], %eax
 ; i386f-NEXT: addb $127, %al
 ; i386f-NEXT: sahf
-; ** FIXME Next line isn't actually necessary. **
-; i386f-NEXT: popl %eax
 ; i386f-NEXT: jne
 
 ; x8664-LABEL: test_intervening_call:
 ; x8664: cmpxchgq
 ; x8664: pushfq
 ; x8664-NEXT: popq [[FLAGS:%.*]]
 ; x8664-NEXT: movq %rax, %rdi
 ; x8664-NEXT: callq bar
 ; x8664-NEXT: pushq [[FLAGS]]
 ; x8664-NEXT: popfq
 ; x8664-NEXT: jne
 
 ; x8664-sahf-LABEL: test_intervening_call:
 ; x8664-sahf: cmpxchgq
 ; x8664-sahf: pushq %rax
 ; x8664-sahf-NEXT: seto %al
 ; x8664-sahf-NEXT: lahf
 ; x8664-sahf-NEXT: movq %rax, [[FLAGS:%.*]]
 ; x8664-sahf-NEXT: popq %rax
 ; x8664-sahf-NEXT: movq %rax, %rdi
 ; x8664-sahf-NEXT: callq bar
-; ** FIXME Next line isn't actually necessary. **
-; x8664-sahf-NEXT: pushq %rax
 ; x8664-sahf-NEXT: movq [[FLAGS]], %rax
 ; x8664-sahf-NEXT: addb $127, %al
 ; x8664-sahf-NEXT: sahf
-; ** FIXME Next line isn't actually necessary. **
-; x8664-sahf-NEXT: popq %rax
 ; x8664-sahf-NEXT: jne
 
   %cx = cmpxchg i64* %foo, i64 %bar, i64 %baz seq_cst seq_cst
   %v = extractvalue { i64, i1 } %cx, 0
   %p = extractvalue { i64, i1 } %cx, 1
   call i32 @bar(i64 %v)
   br i1 %p, label %t, label %f
 
cond.end:
   ret i32 %cond
 }
 
 ; This one is an interesting case because CMOV doesn't have a chain
 ; operand. Naive attempts to limit cmpxchg EFLAGS use are likely to fail here.
 define i32 @test_feed_cmov(i32* %addr, i32 %desired, i32 %new) {
 ; i386-LABEL: test_feed_cmov:
 ; i386: cmpxchgl
-; ** FIXME Next line isn't actually necessary. **
-; i386-NEXT: pushl %eax
 ; i386-NEXT: seto %al
 ; i386-NEXT: lahf
 ; i386-NEXT: movl %eax, [[FLAGS:%.*]]
-; ** FIXME Next line isn't actually necessary. **
-; i386-NEXT: popl %eax
 ; i386-NEXT: calll foo
 ; i386-NEXT: pushl %eax
 ; i386-NEXT: movl [[FLAGS]], %eax
 ; i386-NEXT: addb $127, %al
 ; i386-NEXT: sahf
 ; i386-NEXT: popl %eax
 
 ; i386f-LABEL: test_feed_cmov:
 ; i386f: cmpxchgl
-; ** FIXME Next line isn't actually necessary. **
-; i386f-NEXT: pushl %eax
 ; i386f-NEXT: seto %al
 ; i386f-NEXT: lahf
 ; i386f-NEXT: movl %eax, [[FLAGS:%.*]]
-; ** FIXME Next line isn't actually necessary. **
-; i386f-NEXT: popl %eax
 ; i386f-NEXT: calll foo
 ; i386f-NEXT: pushl %eax
 ; i386f-NEXT: movl [[FLAGS]], %eax
 ; i386f-NEXT: addb $127, %al
 ; i386f-NEXT: sahf
 ; i386f-NEXT: popl %eax
 
 ; x8664-LABEL: test_feed_cmov:
 ; x8664: cmpxchg
 ; x8664: pushfq
 ; x8664-NEXT: popq [[FLAGS:%.*]]
 ; x8664-NEXT: callq foo
 ; x8664-NEXT: pushq [[FLAGS]]
 ; x8664-NEXT: popfq
 
 ; x8664-sahf-LABEL: test_feed_cmov:
 ; x8664-sahf: cmpxchgl
-; ** FIXME Next line isn't actually necessary. **
-; x8664-sahf: pushq %rax
 ; x8664-sahf: seto %al
 ; x8664-sahf-NEXT: lahf
 ; x8664-sahf-NEXT: movq %rax, [[FLAGS:%.*]]
-; ** FIXME Next line isn't actually necessary. **
-; x8664-sahf-NEXT: popq %rax
 ; x8664-sahf-NEXT: callq foo
 ; x8664-sahf-NEXT: pushq %rax
 ; x8664-sahf-NEXT: movq [[FLAGS]], %rax
 ; x8664-sahf-NEXT: addb $127, %al
 ; x8664-sahf-NEXT: sahf
 ; x8664-sahf-NEXT: popq %rax
 
   %res = cmpxchg i32* %addr, i32 %desired, i32 %new seq_cst seq_cst
   %success = extractvalue { i32, i1 } %res, 1
 
   %rhs = call i32 @foo()
 
   %ret = select i1 %success, i32 %new, i32 %rhs
   ret i32 %ret
 }