[PATCH] D31188: [AntiDepBreaker] Do not use getPristineRegs for marking live registers.

[Matthias Braun via Phabricator via llvm-commits]

MatzeB requested changes to this revision.
MatzeB added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp:164
   // callee-saved register that is not saved in the prolog.
-  const MachineFrameInfo &MFI = MF.getFrameInfo();
-  BitVector Pristine = MFI.getPristineRegs(MF);
+  const auto &MFI = MF.getFrameInfo();
+  const auto HasSlot = [this, &MFI](unsigned Reg) {
----------------
Please do not use auto in context where the type is not immediately obvious. (A reader has to lookup that `getFrameInfo()` does return a `MachineFrameInfo` here, it is not immediately obvious). A few more of these below.


================
Comment at: llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp:175
     unsigned Reg = *I;
-    if (!IsReturnBlock && !Pristine.test(Reg)) continue;
-    for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
-      unsigned AliasReg = *AI;
-      State->UnionGroups(AliasReg, 0);
-      KillIndices[AliasReg] = BB->size();
-      DefIndices[AliasReg] = ~0u;
+    if (MFI.isCalleeSavedInfoValid() &&
+        (IsReturnBlock || BB->isLiveIn(Reg) || !HasSlot(Reg))) {
----------------
I don't think this pass will (or should) be used before the prolog epilog inserter so isCalleeSavedInfoValid() should always be true and you could simply assert on it.


================
Comment at: llvm/lib/CodeGen/AggressiveAntiDepBreaker.cpp:176
+    if (MFI.isCalleeSavedInfoValid() &&
+        (IsReturnBlock || BB->isLiveIn(Reg) || !HasSlot(Reg))) {
+      for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
----------------
Adding `isLiveIn()` here looks wrong given that the code is already adding the live ins of the successors above (which is the way to compute the live-outs of this block). This should already be fine if the code is walking the block backwards afterwards.

Seeing this I would assume we walk the blocks backwards and starting with the live-outs so adding the live-ins would not make sense.


================
Comment at: llvm/lib/CodeGen/CriticalAntiDepBreaker.cpp:82-86
+    unsigned Reg = *I;
+    if (MFI.isCalleeSavedInfoValid() &&
+        (IsReturnBlock || BB->isLiveIn(Reg) || !HasSlot(Reg))) {
+      for (MCRegAliasIterator AI(Reg, TRI, true); AI.isValid(); ++AI) {
+        unsigned Reg = *AI;
----------------
see above.


https://reviews.llvm.org/D31188