[PATCH] D75993: [Target][ARM] Improvements to the VPT Block Insertion Pass

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:97
 
+static ARM::PredBlockMask ExpandBlockMask(ARM::PredBlockMask BlockMask,
+                                          unsigned Count,
----------------
Is this only called with a Count of 1 now? If so, can it be simplified.


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:101
+  using PredBlockMask = ARM::PredBlockMask;
+  assert(Count != 0 && Kind != ARMVCC::None);
+  assert(countTrailingZeros((unsigned)BlockMask) != 0 &&
----------------
Add a message for the assert.


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:134
+// Iter didn't point to a predicated instruction.
+static bool SkipPredicatedInstrs(MachineBasicBlock::instr_iterator &Iter,
+                                 MachineBasicBlock::instr_iterator EndIter,
----------------
Maybe name this "AddPredicatedInstruction", as we conceptually "adding" them to the block, as opposed to skipping them. 


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:161
+    if (Iter->definesRegister(ARM::VPR) ||
+        Iter->findRegisterUseOperandIdx(ARM::VPR, true) != -1)
+      return true;
----------------
Can this use killsRegister?


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:177
+  MachineBasicBlock::instr_iterator BlockBeg = Iter;
+  assert(getVPTInstrPredicate(*Iter) == ARMVCC::Then);
+
----------------
Add a message to the assert.


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:201
+    // Try to skip all of the predicated instructions after the VPNOT, stopping
+    // after (4 - VPTThenInstCnt). If we can't skip them all, stop.
+    unsigned ElseInstCnt = 0;
----------------
Was VPTThenInstCnt renamed to something else? BlockSize?


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:248-251
+    if (CurrentPredicate == ARMVCC::Then)
+      CurrentPredicate = ARMVCC::Else;
+    else
+      CurrentPredicate = ARMVCC::Then;
----------------
Maybe just use:
  CurrentPredicate = CurrentPredicate == ARMVCC::Then ? ARMVCC::Else : ARMVCC::Then;


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:273-275
+    // from assembly source or disassembled from object code, you expect to
+    // see a mixture whenever there's a long VPT block. But in code
+    // generation, we hope we'll never generate an Else as input to this pass.
----------------
This doesn't look like it needed to change? Unless the line after is too long too?


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:283-284
 
-    LLVM_DEBUG(dbgs() << "VPT block created for: "; MI->dump());
-    int VPTInstCnt = 1;
-    ARMVCC::VPTCodes NextPred;
-
-    // Look at subsequent instructions, checking if they can be in the same VPT
-    // block.
-    ++MBIter;
-    while (MBIter != EndIter && VPTInstCnt < 4) {
-      NextPred = getVPTInstrPredicate(*MBIter, PredReg);
-      assert(NextPred != ARMVCC::Else &&
-             "VPT block pass does not expect Else preds");
-      if (NextPred != Pred)
-        break;
-      LLVM_DEBUG(dbgs() << "  adding : "; MBIter->dump());
-      ++VPTInstCnt;
-      ++MBIter;
-    };
-
-    unsigned BlockMask = getARMVPTBlockMask(VPTInstCnt);
+    uint64_t BlockMask =
+        (uint64_t)CreateVPTBlock(MBIter, EndIter, DeadInstructions);
 
----------------
Why use a uint64_t?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75993/new/

https://reviews.llvm.org/D75993