[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:134
+// Iter didn't point to a predicated instruction.
+static bool SkipPredicatedInstrs(MachineBasicBlock::instr_iterator &Iter,
+                                 MachineBasicBlock::instr_iterator EndIter,
----------------
Pierre-vh wrote:
> dmgreen wrote:
> > Maybe name this "AddPredicatedInstruction", as we conceptually "adding" them to the block, as opposed to skipping them. 
> I named the function "Skip" because it advances the iterator (it skips the predicated instruction in front of the iterator), this function doesn't really "care" about the block as a whole, so for me calling it "AddPredicatedInstructions" doesn't make sense.
Hmm. "Skip" implied to me that we would not end up doing anything with these instructions. Like we would skip instructions between VPT blocks that are not predicated. We just leave them alone.

How about "StepOver" instead?


================
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);
 
----------------
Pierre-vh wrote:
> dmgreen wrote:
> > Why use a uint64_t?
> It's the type of the argument of `addImm` below.
> Of course I can also use `unsigned` if you prefer, or I can change this to `ARM::PredBlockMask` and convert to `uint64_t` when calling `addImm`.
unsigned is probably fine, it will fit in either case. If the printing is still simple, PredBlockMask would probably be best. Up to you.


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

https://reviews.llvm.org/D75993