[PATCH] D37611: [IfConversion] More simple, correct dead/kill liveness handling

[Matthias Braun via Phabricator via llvm-commits]

MatzeB added a comment.

This is a very good idea!



================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:425
 
+void MachineBasicBlock::removeDeadAndKillFlagsForLiveRegs() {
+  const TargetRegisterInfo *TRI =
----------------
- I'd put this function in LivePhysRegs.h maybe next to computeLiveIns() and not make it a member of MachineBasicBlock.
- How about changing this to also add kill/dead flags (see below) and calling it `recomputeLivenessFlags()`?
- It may or may not be a good idea to break this up into a more flexible function:
  `recomputeLivenessFlagsInRange(LivePhysRegs &LiveRegs, iterator_range<MachineBasicBlock::reverse_iterator> Range);` so that you can apply it to arbitrary ranges inside a basic block given a LivePhysRegs with liveness behind the range. You could still have a convenience function that processes a whole block and uses this internally.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:426
+void MachineBasicBlock::removeDeadAndKillFlagsForLiveRegs() {
+  const TargetRegisterInfo *TRI =
+    getParent()->getSubtarget().getRegisterInfo();
----------------
Try using references for things that cannot be `nullptr`.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:429-434
+  // We walk through the block backwards and start with the registers that
+  // successor BBs say are live.
+  LivePhysRegs LiveRegs;
+  LiveRegs.init(*TRI);
+  for (auto Succ : successors())
+    LiveRegs.addLiveIns(*Succ);
----------------
Better use `LiveRegs.addLiveOutsNoPristines()` instead of doing the loop yourself. It will get some corner cases like return blocks right.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:433
+  LiveRegs.init(*TRI);
+  for (auto Succ : successors())
+    LiveRegs.addLiveIns(*Succ);
----------------
(writing `MachineBasicBlock *` instead of `auto` is friendlier to readers.)


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:437-438
+  for (MachineInstr &MI : make_range(rbegin(), rend())) {
+    if (MI.isDebugValue())
+      continue;
+    for (MIBundleOperands MO(MI); MO.isValid(); ++MO) {
----------------
Are DebugValues allowed to be inside instruction bundles? I'd probably go for if (MO->isDebug()) in the following loop instead of using `MI.isDebugValue()` here.


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:446
+        continue;
+
+      bool IsLive = false;
----------------
Maybe add an `assert(TargetRegisterInfo::isPhysReg(Reg))` here to make things explicit?


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:447-449
+      bool IsLive = false;
+      for (unsigned LiveReg : LiveRegs)
+        IsLive = IsLive || TRI->regsOverlap(Reg, LiveReg);
----------------
This is expensive, and not necessary I think:


================
Comment at: lib/CodeGen/MachineBasicBlock.cpp:454-457
+      if (MO->isKill())
+        MO->setIsKill(false);
+      if (MO->isDead())
+        MO->setIsDead(false);
----------------
How about:
```
if (MO->isDef())
  MO->setIsDead(!LiveRegs.available(MRI, Reg));
if (MO->readsReg())
  MO->setIsKill(!LiveRegs.available(MRI, Reg));
```


https://reviews.llvm.org/D37611