[WinEH] Mark funclet entries and exits as clobbering all registers

[rnk]

Summary

In this implementation, LiveIntervalAnalysis invents a few register
masks on basic block boundaries that preserve no registers. The nice
thing about this is that it prevents the prologue inserter from thinking
it needs to spill all XMM CSRs, because it doesn't see any explicit
physreg defs in the MI.

[MatzeB]

This looks good in general and easier than I suspected :)

In general though I'd like the method which gives you the clobber mask factored out of LiveIntervalAnalysis and moved into MachineBasicBlock (see comment below).

I looked shortly at the register allocators: It seems PBQP, Base, Greedy all use the infrastructure in LiveIntervalAnalysis to check clobbers, RegAllocFast seems to be querying clobbering operands manually and does not use the RegMaskSlots/RegMaskBits parts so it would need some adjustments to work correctly I believe.

lib/CodeGen/LiveIntervalAnalysis.cpp

225

This is fine but I should go into a separate commit (which you can just commit without review).

233

I'm worried that we now have EH specific code in the LiveInterval analysis. I'd rather see this factored out into a MBB.getBeginClobberMask(). This will also make this functionality accessible to register allocators which do not use the RegMaskSlots/RegMaskBits acceleration logic in LiveIntervals.

239

I guess this can go into a separate cleanup patch as well.

244–249

I'm confused by this part. Shouldn't the ReturnBlock end in a return instruction? How can there be successor blocks how can there be anything live across the return instruction that would require this clobbering?

[rnk]

• Move BB mask logic out of line

lib/CodeGen/LiveIntervalAnalysis.cpp

225

OK

244–249

Funclet returns will have successor blocks. There are two cases: the funclet returns the address of the next block that the runtime should resume execution at, or the funclet is a cleanup that returns void and the runtime will run the next cleanup or keep unwinding.

It is possible for the outermost cleanup funclet to have no successors, but it's also not interesting to put a clobber mask there, because the function will be over.

[rnk]

• Fix variable shadowing issue

[rnk]

• Fix copy-pasto

[MatzeB]

LGTM.

The regalloc changes are fine. Adjusting FastRegAlloc can be done in a followup patch, something should obviously be done about the getNoPreservedMask() implementations, but we don't need further review for that.

include/llvm/CodeGen/MachineBasicBlock.h

383 ↗

(On Diff #39454)

I would add an extra comment for each function, something like:

/// Get the clobber mask for the begin of the basic block. Funclets use this ...

/// Get the clobber mask for the end of the basic block. \see getBeginClobberMask()

lib/CodeGen/MachineBasicBlock.cpp

1322 ↗

(On Diff #39454)

As discussed.

lib/Target/ARM/ARMBaseRegisterInfo.h

97

There is no implementation for this included in this patch (same for the other Targets).

[JosephTremoulet]

Thanks for fixing this!

Should you also be removing the FIXME code in X86ISelLowering.cpp (~line 3445) that sets the noPreserved mask on invokes (with 32-bit funclet personalities)? I don't see that here.

[rnk]

In D14407#283135, @JosephTremoulet wrote:

Should you also be removing the FIXME code in X86ISelLowering.cpp (~line 3445) that sets the noPreserved mask on invokes (with 32-bit funclet personalities)? I don't see that here.

I can't remove that yet because it also informs PEI that it needs to push all non-volatile registers in the prologue. We *don't* want that behavior for x64, because the runtime takes care of it for us, and redundantly saving all callee-saved XMM registers is expensive.

I think the right way to handle that is to put a nothing-preserved mask on EH_RESTORE instructions, which corresponds to where control rejoins an outer scope after a CATCHRET. That way, 32-bit functions using only cleanups will not need to spill all CSRs.

lib/Target/ARM/ARMBaseRegisterInfo.h

97

The implementations already exist, I only made the method virtual. I'll make the default behavior be fatal error.

[rnk]

• Final version

[JosephTremoulet]

In D14407#283723, @rnk wrote:

In D14407#283135, @JosephTremoulet wrote:

Should you also be removing the FIXME code in X86ISelLowering.cpp (~line 3445) that sets the noPreserved mask on invokes (with 32-bit funclet personalities)? I don't see that here.

I can't remove that yet because it also informs PEI that it needs to push all non-volatile registers in the prologue. We *don't* want that behavior for x64, because the runtime takes care of it for us, and redundantly saving all callee-saved XMM registers is expensive.

I think the right way to handle that is to put a nothing-preserved mask on EH_RESTORE instructions, which corresponds to where control rejoins an outer scope after a CATCHRET. That way, 32-bit functions using only cleanups will not need to spill all CSRs.

Oops -- I didn't realize that the 32-bit runtime doesn't restore them for us on "catchret". What you say makes sense, thanks for explaining.

include/llvm/Target/TargetRegisterInfo.h

public:
   /// registers.
   ///
   virtual const uint32_t *getCallPreservedMask(const MachineFunction &MF,
                                                CallingConv::ID) const {
     // The default mask clobbers everything.  All targets should override.
     return nullptr;
   }
 
+  // Return a register mask that clobbers everything.
+  virtual const uint32_t *getNoPreservedMask() const {
+    return nullptr;
+  }
+
   /// Return all the call-preserved register masks defined for this target.
   virtual ArrayRef<const uint32_t *> getRegMasks() const = 0;
   virtual ArrayRef<const char *> getRegMaskNames() const = 0;
 
   /// getReservedRegs - Returns a bitset indexed by physical register number
   /// indicating if a register is a special register that has particular uses
   /// and should be considered unavailable at all times, e.g. SP, RA. This is
   /// used by register scavenger to determine what registers are free.

lib/CodeGen/LiveIntervalAnalysis.cpp

for (unsigned i = 0, e = MRI->getNumVirtRegs(); i != e; ++i) {
     createAndComputeVirtRegInterval(Reg);
   }
 }
 
 void LiveIntervals::computeRegMasks() {
   RegMaskBlocks.resize(MF->getNumBlockIDs());
 
   // Find all instructions with regmask operands.
-  for (MachineFunction::iterator MBBI = MF->begin(), E = MF->end();
-       MBBI != E; ++MBBI) {
-    MachineBasicBlock *MBB = &*MBBI;
-    std::pair<unsigned, unsigned> &RMB = RegMaskBlocks[MBB->getNumber()];
+  for (MachineBasicBlock &MBB : *MF) {
+    std::pair<unsigned, unsigned> &RMB = RegMaskBlocks[MBB.getNumber()];
     RMB.first = RegMaskSlots.size();
-    for (MachineBasicBlock::iterator MI = MBB->begin(), ME = MBB->end();
-         MI != ME; ++MI)
-      for (const MachineOperand &MO : MI->operands()) {
+
+    // Windows EH funclets don't allow registers to be live in.
+    if (MBB.isEHFuncletEntry()) {
+      RegMaskSlots.push_back(Indexes->getMBBStartIdx(&MBB));
+      RegMaskBits.push_back(TRI->getNoPreservedMask());
+    }
+
+    for (MachineInstr &MI : MBB) {
+      for (const MachineOperand &MO : MI.operands()) {
         if (!MO.isRegMask())
           continue;
-          RegMaskSlots.push_back(Indexes->getInstructionIndex(MI).getRegSlot());
-          RegMaskBits.push_back(MO.getRegMask());
+        RegMaskSlots.push_back(Indexes->getInstructionIndex(&MI).getRegSlot());
       }
+    }
+
+    // If we see a return block with successors, this must be a funclet return.
+    // Add an extra preserves-nothing constraint to the end of the block.
+    if (MBB.isReturnBlock() && !MBB.succ_empty()) {
+      RegMaskSlots.push_back(Indexes->getMBBEndIdx(&MBB));
+      RegMaskBits.push_back(TRI->getNoPreservedMask());
+    }
+
     // Compute the number of register mask instructions in this block.
     RMB.second = RegMaskSlots.size() - RMB.first;
   }
 }
 
 //===----------------------------------------------------------------------===//
 //                           Register Unit Liveness
 //===----------------------------------------------------------------------===//

lib/Target/ARM/ARMBaseRegisterInfo.h

protected:
   // Return the opcode that implements 'Op', or 0 if no opcode
   unsigned getOpcode(int Op) const;
 
 public:
   /// Code Generation virtual methods...
   const MCPhysReg *getCalleeSavedRegs(const MachineFunction *MF) const override;
   const uint32_t *getCallPreservedMask(const MachineFunction &MF,
                                        CallingConv::ID) const override;
-  const uint32_t *getNoPreservedMask() const;
+  const uint32_t *getNoPreservedMask() const override;
 
   /// getThisReturnPreservedMask - Returns a call preserved mask specific to the
   /// case that 'returned' is on an i32 first argument if the calling convention
   /// is one that can (partially) model this attribute with a preserved mask
   /// (i.e. it is a calling convention that uses the same register for the first
   /// i32 argument and an i32 return value)
   ///
   /// Should return NULL in the case that the calling convention does not have

lib/Target/PowerPC/PPCRegisterInfo.h

public:
   const TargetRegisterClass *
   getLargestLegalSuperClass(const TargetRegisterClass *RC,
                             const MachineFunction &MF) const override;
 
   /// Code Generation virtual methods...
   const MCPhysReg *getCalleeSavedRegs(const MachineFunction *MF) const override;
   const uint32_t *getCallPreservedMask(const MachineFunction &MF,
                                        CallingConv::ID CC) const override;
-  const uint32_t *getNoPreservedMask() const;
+  const uint32_t *getNoPreservedMask() const override;
 
   void adjustStackMapLiveOutMask(uint32_t *Mask) const override;
 
   BitVector getReservedRegs(const MachineFunction &MF) const override;
 
   /// We require the register scavenger.
   bool requiresRegisterScavenging(const MachineFunction &MF) const override {
     return true;

lib/Target/X86/X86RegisterInfo.h

unsigned getRegPressureLimit(const TargetRegisterClass *RC,
                                MachineFunction &MF) const override;
 
   /// getCalleeSavedRegs - Return a null-terminated list of all of the
   /// callee-save registers on this target.
   const MCPhysReg *
   getCalleeSavedRegs(const MachineFunction* MF) const override;
   const uint32_t *getCallPreservedMask(const MachineFunction &MF,
                                        CallingConv::ID) const override;
-  const uint32_t *getNoPreservedMask() const;
+  const uint32_t *getNoPreservedMask() const override;
 
   /// getReservedRegs - Returns a bitset indexed by physical register number
   /// indicating if a register is a special register that has particular uses and
   /// should be considered unavailable at all times, e.g. SP, RA. This is used by
   /// register scavenger to determine what registers are free.
   BitVector getReservedRegs(const MachineFunction &MF) const override;
 
   void adjustStackMapLiveOutMask(uint32_t *Mask) const override;

test/CodeGen/X86/catchpad-regmask.ll

+; RUN: llc < %s | FileCheck %s
+
+; Based on this code:
+;
+; extern "C" int array[4];
+; extern "C" void global_array(int idx1, int idx2, int idx3) {
+;   try {
+;     array[idx1] = 111;
+;     throw;
+;   } catch (...) {
+;     array[idx2] = 222;
+;   }
+;   array[idx3] = 333;
+; }
+; extern "C" __declspec(dllimport) int imported;
+; extern "C" void access_imported() {
+;   try {
+;     imported = 111;
+;     throw;
+;   } catch (...) {
+;     imported = 222;
+;   }
+;   imported = 333;
+; }
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc18.0.0"
+
+%eh.ThrowInfo = type { i32, i32, i32, i32 }
+
+@array = external global [4 x i32], align 16
+@imported = external dllimport global i32, align 4
+
+; Function Attrs: uwtable
+define void @global_array(i32 %idx1, i32 %idx2, i32 %idx3) #0 personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
+  %idxprom = sext i32 %idx1 to i64
+  %arrayidx = getelementptr inbounds [4 x i32], [4 x i32]* @array, i64 0, i64 %idxprom
+  store i32 111, i32* %arrayidx, align 4, !tbaa !2
+  invoke void @_CxxThrowException(i8* null, %eh.ThrowInfo* null) #1
+          to label %unreachable unwind label %catch.dispatch
+
+catch.dispatch:                                   ; preds = %entry
+  %0 = catchpad [i8* null, i32 64, i8* null]
+          to label %catch unwind label %catchendblock
+
+catch:                                            ; preds = %catch.dispatch
+  %idxprom1 = sext i32 %idx2 to i64
+  %arrayidx2 = getelementptr inbounds [4 x i32], [4 x i32]* @array, i64 0, i64 %idxprom1
+  store i32 222, i32* %arrayidx2, align 4, !tbaa !2
+  catchret %0 to label %try.cont
+
+try.cont:                                         ; preds = %catch
+  %idxprom3 = sext i32 %idx3 to i64
+  %arrayidx4 = getelementptr inbounds [4 x i32], [4 x i32]* @array, i64 0, i64 %idxprom3
+  store i32 333, i32* %arrayidx4, align 4, !tbaa !2
+  ret void
+
+catchendblock:                                    ; preds = %catch.dispatch
+  catchendpad unwind to caller
+
+unreachable:                                      ; preds = %entry
+  unreachable
+}
+
+; CHECK-LABEL: global_array: # @global_array
+; CHECK: pushq %rbp
+;   First array access
+; CHECK: movslq  %ecx, %[[idx:[^ ]*]]
+; CHECK: leaq    array(%rip), %[[base:[^ ]*]]
+; CHECK: movl    $111, (%[[base]],%[[idx]],4)
+; Might throw an exception and return to below...
+; CHECK: callq   _CxxThrowException
+;   Third array access must remat the address of array
+; CHECK: movslq  {{.*}}, %[[idx:[^ ]*]]
+; CHECK: leaq    array(%rip), %[[base:[^ ]*]]
+; CHECK: movl    $333, (%[[base]],%[[idx]],4)
+; CHECK: popq %rbp
+; CHECK: retq
+
+; CHECK: "?catch$2@?0?global_array@4HA":
+; CHECK: pushq   %rbp
+; CHECK: movslq  {{.*}}, %[[idx:[^ ]*]]
+; CHECK: leaq    array(%rip), %[[base:[^ ]*]]
+; CHECK: movl    $222, (%[[base]],%[[idx]],4)
+; CHECK: popq    %rbp
+; CHECK: retq                            # CATCHRET
+
+declare void @_CxxThrowException(i8*, %eh.ThrowInfo*)
+
+declare i32 @__CxxFrameHandler3(...)
+
+; Function Attrs: uwtable
+define void @access_imported() #0 personality i8* bitcast (i32 (...)* @__CxxFrameHandler3 to i8*) {
+entry:
+  store i32 111, i32* @imported, align 4, !tbaa !2
+  invoke void @_CxxThrowException(i8* null, %eh.ThrowInfo* null) #1
+          to label %unreachable unwind label %catch.dispatch
+
+catch.dispatch:                                   ; preds = %entry
+  %0 = catchpad [i8* null, i32 64, i8* null]
+          to label %catch unwind label %catchendblock
+
+catch:                                            ; preds = %catch.dispatch
+  store i32 222, i32* @imported, align 4, !tbaa !2
+  catchret %0 to label %try.cont
+
+try.cont:                                         ; preds = %catch
+  store i32 333, i32* @imported, align 4, !tbaa !2
+  ret void
+
+catchendblock:                                    ; preds = %catch.dispatch
+  catchendpad unwind to caller
+
+unreachable:                                      ; preds = %entry
+  unreachable
+}
+
+; CHECK-LABEL: access_imported: # @access_imported
+; CHECK: pushq %rbp
+; CHECK: movq    __imp_imported(%rip), %[[base:[^ ]*]]
+; CHECK: movl    $111, (%[[base]])
+; Might throw an exception and return to below...
+; CHECK: callq   _CxxThrowException
+;   Third access must reload the address of imported
+; CHECK: movq    __imp_imported(%rip), %[[base:[^ ]*]]
+; CHECK: movl    $333, (%[[base]])
+; CHECK: popq %rbp
+; CHECK: retq
+
+; CHECK: "?catch$2@?0?access_imported@4HA":
+; CHECK: pushq   %rbp
+; CHECK: movq    __imp_imported(%rip), %[[base:[^ ]*]]
+; CHECK: movl    $222, (%[[base]])
+; CHECK: popq    %rbp
+; CHECK: retq                            # CATCHRET
+
+
+attributes #0 = { uwtable "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { noreturn }
+
+!llvm.module.flags = !{!0}
+!llvm.ident = !{!1}
+
+!0 = !{i32 1, !"PIC Level", i32 2}
+!1 = !{!"clang version 3.8.0 "}
+!2 = !{!3, !3, i64 0}
+!3 = !{!"int", !4, i64 0}
+!4 = !{!"omnipotent char", !5, i64 0}
+!5 = !{!"Simple C/C++ TBAA"}

test/CodeGen/X86/cleanuppad-large-codemodel.ll

+; RUN: llc -mtriple=x86_64-pc-windows-msvc -code-model=large -o - < %s | FileCheck %s
+
+declare i32 @__CxxFrameHandler3(...)
+
+declare void @bar()
+
+define void @foo() personality i32 (...)* @__CxxFrameHandler3 {
+entry:
+  invoke void @bar()
+    to label %exit unwind label %cleanup
+cleanup:
+  %c = cleanuppad []
+  call void @bar()
+  cleanupret %c unwind to caller
+exit:
+  ret void
+}
+
+; CHECK: foo: # @foo
+; CHECK: movabsq $bar, %[[reg:[^ ]*]]
+; CHECK: callq *%[[reg]]
+; CHECK: retq
+
+; CHECK: "?dtor$2@?0?foo@4HA":
+; CHECK: movabsq $bar, %[[reg:[^ ]*]]
+; CHECK: callq *%[[reg]]
+; CHECK: retq                            # CLEANUPRET