DBG_VALUE insertion for spills breaks bundles

[saurabhverma]

Summary

• follow up from llvm-dev email thread, summary below **
• Please note that this was discovered with a complex VLIW scheduling testcase in an out-of-tree target. I haven't managed to get an equivalent testcase for it with the upstream targets, but I am posting this patch to be taken up based on the fact that this error can (and does) result in inconsistent bundle flags and broken schedules. Even without the testcase, the code for DBG_VALUE insertion can be checked to see that this error can occur in the scenario described below **

http://lists.llvm.org/pipermail/llvm-dev/2018-January/120076.html

The insertion of DBG_VALUE instructions for spills does not seem to be handling insert locations inside bundles well. If the spill instruction is part of a bundle, the new DBG_VALUE is inserted after it, but does not have the bundling flags set. This essentially means that if we start with a set of bundled instructions:

MI1 [BundledSucc=true, BundledPred=false]
MI2 [BundledSucc=false, BundledPred=true]

Where MI1 is a spill, and MI2 is a different instruction, after ExtendRanges we end up with

MI1 [BundledSucc=true, BundledPred=false]
DBG_VALUE MI [BundledSucc=false, BundledPred=false]
MI2 [BundledSucc=false, BundledPred=true]

Since this happens after the final instruction scheduling, it results in broken schedules.
The patch checks if the DBG_VALUE node is inserted in the middle of a bundle and makes sure that its Bundle flags are set correctly, if so.

[aprantl]

lib/CodeGen/LiveDebugValues.cpp

676

If there is really no way to test this with in-tree targets, we should at the very least have a detailed comment here explaining why this is implemented this way. Could you please add a comment that explains the rationale like in the review description here?

[saurabhverma]

Adding descriptive comment for the change

[aprantl]

lib/CodeGen/LiveDebugValues.cpp

687

clang-format please

[JDevlieghere]

I don't know if this makes any sense, but could we maybe test this by constructing a bundled spill in a MIR file for an in-tree target?

cc @thegameg

[thegameg]

In D41864#971308, @JDevlieghere wrote:

I don't know if this makes any sense, but could we maybe test this by constructing a bundled spill in a MIR file for an in-tree target?

cc @thegameg

Just noticed that we don't have any documentation on bundled instructions in MIRLangRef. Here is a patch: https://reviews.llvm.org/D41872.

[saurabhverma]

Format fixes . Now passes clang-format

[saurabhverma]

Thanks for reviewing and approving the change. I don't have commit access so will request you to commit it please. I have taken care of the formatting and it now clears clang-format.

[aprantl]

@saurabhverma
have you looked into JDevlieghere's suggestion?

[saurabhverma]

In D41864#972496, @aprantl wrote:

@saurabhverma
have you looked into JDevlieghere's suggestion?

Hi @aprantl , Yes. that was the approach I had tried to generate the testcase, but did not have much success. Will give it another shot.

lib/CodeGen/LiveDebugValues.cpp

while (!Worklist.empty()) {
         // Now that we have started to extend ranges across BBs we need to
         // examine spill instructions to see whether they spill registers that
         // correspond to user variables.
         for (auto &MI : *MBB)
           OLChanged |= transfer(MI, OpenRanges, OutLocs, VarLocIDs, Spills,
                                 /*transferSpills=*/true);
 
         // Add any DBG_VALUE instructions necessitated by spills.
-        for (auto &SP : Spills)
-          MBB->insertAfter(MachineBasicBlock::iterator(*SP.SpillInst),
-                           SP.DebugInst);
+        for (auto &SP : Spills) {
+          MachineInstr *InsertPoint = SP.SpillInst;
+          MachineInstr *DebugInst = SP.DebugInst;
+          MBB->insertAfter(MachineBasicBlock::iterator(*InsertPoint),
+                           DebugInst);
+          // If the spill is part of an instruction bundle, we need to make sure
+          // that the newly inserted DBG_VALUE instruction also has the bundle
+          // flags set. Otherwise we might end up with incorrect sequences like:
+          //
+          //        MI1 [BundledSucc=true, BundledPred=false]
+          //        DBG_VALUE MI [BundledSucc=false, BundledPred=false]
+          //        MI2 [BundledSucc=false, BundledPred=true]
+          if (InsertPoint->isBundledWithSucc()) {
+            DebugInst->setFlag(MachineInstr::BundledSucc);
+            DebugInst->setFlag(MachineInstr::BundledPred);
+          }
+        }
         Spills.clear();
 
         DEBUG(printVarLocInMBB(MF, OutLocs, VarLocIDs,
                                "OutLocs after propagating", dbgs()));
         DEBUG(printVarLocInMBB(MF, InLocs, VarLocIDs,
                                "InLocs after propagating", dbgs()));
 
         if (OLChanged) {