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

[Matthias Braun via llvm-commits]

MatzeB added a comment.

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.


================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:225
@@ -224,5 +224,3 @@
   // 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()];
----------------
This is fine but I should go into a separate commit (which you can just commit without review).

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:233
@@ +232,3 @@
+      RegMaskBits.push_back(TRI->getNoPreservedMask());
+    }
+
----------------
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.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:239
@@ -234,3 +238,3 @@
           continue;
-          RegMaskSlots.push_back(Indexes->getInstructionIndex(MI).getRegSlot());
-          RegMaskBits.push_back(MO.getRegMask());
+        RegMaskSlots.push_back(Indexes->getInstructionIndex(&MI).getRegSlot());
+        RegMaskBits.push_back(MO.getRegMask());
----------------
I guess this can go into a separate cleanup patch as well.

================
Comment at: lib/CodeGen/LiveIntervalAnalysis.cpp:244-249
@@ +243,8 @@
+
+    // 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());
+    }
+
----------------
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?


http://reviews.llvm.org/D14407