[X86] Detect if EFLAGs is live across XBEGIN pseudo instruction. Add it as livein to the basic blocks created when expanding the pseudo

[craig.topper]

Summary

XBEGIN causes several based blocks to be inserted. If flags are live across it we need to make eflags live in the new basic blocks to avoid machine verifier errors.

Is there a better way to do this than just scanning the rest of the basic block of uses or defs?

Fixes PR46827

[djtodoro]

@craig.topper Thanks for fixing this!

llvm/lib/Target/X86/X86ISelLowering.cpp

31005

So this situation can happen *only* for $EFLAGS?

[craig.topper]

llvm/lib/Target/X86/X86ISelLowering.cpp

31005

I don't know for sure. I copied the code from EmitLoweredSelect where expanding CMOV pseudos has the same issue.

[djtodoro]

llvm/lib/Target/X86/X86ISelLowering.cpp

31005

Can we make a static function and reuse the interface? I guess we can face this in some other pseudo instructions expanding as well.

[craig.topper]

Share code with CMOV pseudo expansion. Also check for liveins of the successor BBs which was done by cmov.

[craig.topper]

Refactor code to just return false when we find a def. Then we don't have to check explicitly for end of the block. If we fall out of the loop then we must have reached the end of the block.

[ivanbaev]

Hi Craig, thank you for working on this bug.

1. * IR Dump Before Finalize ISel and expand pseudo-instructions *:
2. Machine code for function wobble.12: IsSSA, TracksLiveness

bb.0.bb107:

successors: %bb.1(0x40000000), %bb.2(0x40000000); %bb.1(50.00%), %bb.2(50.00%)

%0:gr32 = MOV32rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.0, align 16)
%1:gr32 = SUB32ri8 %0:gr32(tied-def 0), 1, implicit-def $eflags
%2:gr32 = XBEGIN
JCC_1 %bb.2, 5, implicit $eflags
JMP_1 %bb.1

...

---

Given the IR above for the test, an alternative liveness check for eflags is to iterate backward.
Look for a def of eflag, and if we reach the start of the basic block: check BB->isLiveIn(X86::EFLAGS)) and avoid iterating over successors of BB as in the proposed fix.

As optimization add EFLAGS only to sinkMBB
sinkMBB->addLiveIn(X86::EFLAGS);

The proposed fix is good too. It just seems to me that connecting a def to its uses is more natural.

[craig.topper]

In D84479#2173864, @ivanbaev wrote:

Hi Craig, thank you for working on this bug.

1. * IR Dump Before Finalize ISel and expand pseudo-instructions *:
2. Machine code for function wobble.12: IsSSA, TracksLiveness

bb.0.bb107:

successors: %bb.1(0x40000000), %bb.2(0x40000000); %bb.1(50.00%), %bb.2(50.00%)

%0:gr32 = MOV32rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.0, align 16)
%1:gr32 = SUB32ri8 %0:gr32(tied-def 0), 1, implicit-def $eflags
%2:gr32 = XBEGIN
JCC_1 %bb.2, 5, implicit $eflags
JMP_1 %bb.1

...

---

Given the IR above for the test, an alternative liveness check for eflags is to iterate backward.
Look for a def of eflag, and if we reach the start of the basic block: check BB->isLiveIn(X86::EFLAGS)) and avoid iterating over successors of BB as in the proposed fix.

We'd have to also use the kill flag for any users we find before we find the def? We shouldn't make it livein if it was already killed.

As optimization add EFLAGS only to sinkMBB
sinkMBB->addLiveIn(X86::EFLAGS);

Can we have it live in in sinkMBB without it being livein in the predecessors in sinkMBB?

The proposed fix is good too. It just seems to me that connecting a def to its uses is more natural.

[ivanbaev]

Yes, check for mi.killsRegister(X86::EFLAGS)) before a def (in backward order).
The instructions in mainMBB and fallBB during xbegin() expansion do not deal with EFLAGS. If a subsequent pass makes changes in mainMBB and fallBB that might impact the liveness of EFLAGS, that pass should make proper updates. Having

mainMBB->addLiveIn(X86::EFLAGS);
fallMBB->addLiveIn(X86::EFLAGS);

is good too.

[craig.topper]

In D84479#2173935, @ivanbaev wrote:

Yes, check for mi.killsRegister(X86::EFLAGS)) before a def (in backward order).
The instructions in mainMBB and fallBB during xbegin() expansion do not deal with EFLAGS. If a subsequent pass makes changes in mainMBB and fallBB that might impact the liveness of EFLAGS, that pass should make proper updates. Having

mainMBB->addLiveIn(X86::EFLAGS);
fallMBB->addLiveIn(X86::EFLAGS);

is good too.

Removing the addLiveIn from mainMBB/fallMBB gives

*** Bad machine code: Live in register not found to be live out from predecessor. ***
- function:    wobble.12
- basic block: %bb.5 bb107 (0x7f8863865bc0)
EFLAGS not found to be live out from %bb.3

*** Bad machine code: Live in register not found to be live out from predecessor. ***
- function:    wobble.12
- basic block: %bb.5 bb107 (0x7f8863865bc0)
EFLAGS not found to be live out from %bb.4
LLVM ERROR: Found 2 machine code errors.

[ivanbaev]

Thank you for checking on mainMBB and fallBB. Which direction: forward or backward - do you prefer for isEFLAGSLiveAfter()?

[craig.topper]

In D84479#2174059, @ivanbaev wrote:

Thank you for checking on mainMBB and fallBB. Which direction: forward or backward - do you prefer for isEFLAGSLiveAfter()?

I think I'd prefer to keep the forward direction. The code is shared with cmov handling which is also fixing incorrect kill flags on the cmov pseudo instruction.

[ivanbaev]

I don't have strong preference for bwd direction. Let's get this fix in. Are all tests and sanities passing?

[craig.topper]

In D84479#2174882, @ivanbaev wrote:

I don't have strong preference for bwd direction. Let's get this fix in. Are all tests and sanities passing?

ninja check-llvm passes

[djtodoro]

@craig.topper Thanks for addressing the comments, lgtm now.

[ivanbaev]

LGTM. How to change NeedRevision flag?

llvm/lib/Target/X86/X86ISelLowering.cpp

bool X86TargetLowering::areJTsAllowed(const Function *Fn) const {
   // Otherwise, fallback on the generic logic.
   return TargetLowering::areJTsAllowed(Fn);
 }
 
 //===----------------------------------------------------------------------===//
 //                           X86 Scheduler Hooks
 //===----------------------------------------------------------------------===//
 
+// Returns true if EFLAG is consumed after this iterator in the rest of the
+// basic block or any successors of the basic block.
+static bool isEFLAGSLiveAfter(MachineBasicBlock::iterator Itr,
+                              MachineBasicBlock *BB) {
+  // Scan forward through BB for a use/def of EFLAGS.
+  for (MachineBasicBlock::iterator miI = std::next(Itr), miE = BB->end();
+         miI != miE; ++miI) {
+    const MachineInstr& mi = *miI;
+    if (mi.readsRegister(X86::EFLAGS))
+      return true;
+    // If we found a def, we can stop searching.
+    if (mi.definesRegister(X86::EFLAGS))
+      return false;
+  }
+
+  // If we hit the end of the block, check whether EFLAGS is live into a
+  // successor.
+  for (MachineBasicBlock::succ_iterator sItr = BB->succ_begin(),
+                                        sEnd = BB->succ_end();
+       sItr != sEnd; ++sItr) {
+    MachineBasicBlock* succ = *sItr;
+    if (succ->isLiveIn(X86::EFLAGS))
+      return true;
+  }
+
+  return false;
+}
+
 /// Utility function to emit xbegin specifying the start of an RTM region.
 static MachineBasicBlock *emitXBegin(MachineInstr &MI, MachineBasicBlock *MBB,
                                      const TargetInstrInfo *TII) {
   DebugLoc DL = MI.getDebugLoc();
 
   const BasicBlock *BB = MBB->getBasicBlock();
   MachineFunction::iterator I = ++MBB->getIterator();
 
   // For the v = xbegin(), we generate
   //
   // thisMBB:
   //  xbegin sinkMBB
   //
   // mainMBB:
   //  s0 = -1
   //
   // fallBB:
   //  eax = # XABORT_DEF
   //  s1 = eax
   //
   // sinkMBB:
   //  v = phi(s0/mainBB, s1/fallBB)
 
   MachineBasicBlock *thisMBB = MBB;
   MachineFunction *MF = MBB->getParent();
   MachineBasicBlock *mainMBB = MF->CreateMachineBasicBlock(BB);
   MachineBasicBlock *fallMBB = MF->CreateMachineBasicBlock(BB);
   MachineBasicBlock *sinkMBB = MF->CreateMachineBasicBlock(BB);
   MF->insert(I, mainMBB);
   MF->insert(I, fallMBB);
   MF->insert(I, sinkMBB);
 
+  if (isEFLAGSLiveAfter(MI, MBB)) {
+    mainMBB->addLiveIn(X86::EFLAGS);
+    fallMBB->addLiveIn(X86::EFLAGS);
+    sinkMBB->addLiveIn(X86::EFLAGS);
+  }
+
   // Transfer the remainder of BB and its successor edges to sinkMBB.
   sinkMBB->splice(sinkMBB->begin(), MBB,
                   std::next(MachineBasicBlock::iterator(MI)), MBB->end());
   sinkMBB->transferSuccessorsAndUpdatePHIs(MBB);
 
   MachineRegisterInfo &MRI = MF->getRegInfo();
   Register DstReg = MI.getOperand(0).getReg();
   const TargetRegisterClass *RC = MRI.getRegClass(DstReg);

 // The EFLAGS operand of SelectItr might be missing a kill marker
 // because there were multiple uses of EFLAGS, and ISel didn't know
 // which to mark. Figure out whether SelectItr should have had a
 // kill marker, and set it if it should. Returns the correct kill
 // marker value.
 static bool checkAndUpdateEFLAGSKill(MachineBasicBlock::iterator SelectItr,
                                      MachineBasicBlock* BB,
                                      const TargetRegisterInfo* TRI) {
-  // Scan forward through BB for a use/def of EFLAGS.
-  MachineBasicBlock::iterator miI(std::next(SelectItr));
-  for (MachineBasicBlock::iterator miE = BB->end(); miI != miE; ++miI) {
-    const MachineInstr& mi = *miI;
-    if (mi.readsRegister(X86::EFLAGS))
-      return false;
-    if (mi.definesRegister(X86::EFLAGS))
-      break; // Should have kill-flag - update below.
-  }
-
-  // If we hit the end of the block, check whether EFLAGS is live into a
-  // successor.
-  if (miI == BB->end()) {
-    for (MachineBasicBlock::succ_iterator sItr = BB->succ_begin(),
-                                          sEnd = BB->succ_end();
-         sItr != sEnd; ++sItr) {
-      MachineBasicBlock* succ = *sItr;
-      if (succ->isLiveIn(X86::EFLAGS))
-        return false;
-    }
-  }
+  if (isEFLAGSLiveAfter(SelectItr, BB))
 
   // We found a def, or hit the end of the basic block and EFLAGS wasn't live
   // out. SelectMI should have a kill flag on EFLAGS.
   SelectItr->addRegisterKilled(X86::EFLAGS, TRI);
   return true;
 }
 
 // Return true if it is OK for this CMOV pseudo-opcode to be cascaded

llvm/test/CodeGen/X86/pr46827.ll

+; RUN: llc < %s -mtriple=i686-pc-linux -mattr=+rtm -verify-machineinstrs -stop-after=finalize-isel | FileCheck %s
+
+; CHECK: body:             |
+; CHECK:   bb.0.bb107:
+; CHECK:     successors: %bb.3(0x40000000), %bb.4(0x40000000)
+; CHECK:     %0:gr32 = MOV32rm %fixed-stack.0, 1, $noreg, 0, $noreg :: (load 4 from %fixed-stack.0, align 16)
+; CHECK:     %1:gr32 = SUB32ri8 %0, 1, implicit-def $eflags
+; CHECK:     XBEGIN_4 %bb.4, implicit-def $eax
+; CHECK:   bb.3.bb107:
+; CHECK:     successors: %bb.5(0x80000000)
+; CHECK:     liveins: $eflags
+; CHECK:     %3:gr32 = MOV32ri -1
+; CHECK:     JMP_1 %bb.5
+; CHECK:   bb.4.bb107:
+; CHECK:     successors: %bb.5(0x80000000)
+; CHECK:     liveins: $eflags
+; CHECK:     XABORT_DEF implicit-def $eax
+; CHECK:     %4:gr32 = COPY $eax
+; CHECK:   bb.5.bb107:
+; CHECK:     successors: %bb.1(0x40000000), %bb.2(0x40000000)
+; CHECK:     liveins: $eflags
+; CHECK:     %2:gr32 = PHI %3, %bb.3, %4, %bb.4
+; CHECK:     JCC_1 %bb.2, 5, implicit $eflags
+; CHECK:     JMP_1 %bb.1
+
+declare i32 @llvm.x86.xbegin() #0
+
+define void @wobble.12(i32 %tmp116) {
+bb107:                                            ; preds = %bb42
+  %tmp117 = icmp eq i32 %tmp116, 1
+  %tmp127 = tail call i32 @llvm.x86.xbegin() #0
+  br i1 %tmp117, label %bb129, label %bb250
+
+bb129:                                            ; preds = %bb107
+  unreachable
+
+bb250:                                            ; preds = %bb107
+  unreachable
+}