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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh marked 11 inline comments as done.
Pierre-vh added a comment.

(I've marked some comments as Done because I changed those locally, I'll update the diff once the review is complete (2 comments left))



================
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,
----------------
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.


================
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;
----------------
dmgreen wrote:
> Was VPTThenInstCnt renamed to something else? BlockSize?
It's indeed "BlockSize" now, I'll change this comment.


================
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.
----------------
dmgreen wrote:
> This doesn't look like it needed to change? Unless the line after is too long too?
You are right, it doesn't need to change. It was probably changed by mistake by me or clang-format.


================
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);
 
----------------
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`.


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

https://reviews.llvm.org/D75993