[PATCH] D71107: [ARM][MVE] Tail predicate in the presence of vcmp

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

Yep, that's a lot of changes... :-)
I haven't read or digested everything yet, so this a first round of nits.



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:51
+
+  class VPTBlock {
+    std::unique_ptr<PredicatedMI> VPST;
----------------
Can you add some comments what a VPTBlock is? For example, is it exactly the same as in the ArmARM, or slightly different? Are there any restrictions on the number of instructions in this block, so probably good to assert that in `addInst`?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:70
+
+    bool HasNonUniformPredicate() const {
+      return Divergent != nullptr;
----------------
I think this one can benefit from some comments too. I.e., some comments on how a non-uniform predicate looks like in this context would be good.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:152
 
+    SmallVectorImpl<VPTBlock> &getBlocks() { return VPTBlocks; }
+
----------------
nit: `getVPTBlocks`?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:382
+  // All predication within the loop should be based on vctp. If the block
+  // isn't predicatd on entry, check whether the vctp is within the block
+  // and that all other instructions are then predicated on it.
----------------
typo: predicatd


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:463
+bool LowOverheadLoop::RecordVPTBlocks(MachineInstr* MI) {
+  if (IsVCTP(MI)) {
+    if (VCTP)
----------------
This function is doing a lot of things. Can this first if-else if be rewritten (using some helpers) to something along the lines of this:

  if (!SetVCTP(MI) || IsVPT(MI))
    return false;
  else if (shouldCreateNewVPTBlock(MI))
    return true;


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:474
+
+  unsigned VPROpNum = MI->getNumOperands() - 1;
+  if (MI->getOperand(VPROpNum).isReg() &&
----------------
And this block is doing adding the instruction to the block, so can this e.g. be `addInstToVPTBlock()`?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:478
+      MI->getOperand(VPROpNum).isUse()) {
+    // If this instruction is predicated by VPR, it will be it's last
+    // operand.  Also check that it's only 'Then' predicated.
----------------
typo: it's


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:496
+      CurrentPredicate.insert(MI);
+    else if (i != VPROpNum) {
+      LLVM_DEBUG(dbgs() << "ARM Loops: Found instruction using vpr: " << *MI);
----------------
Ignore this comment if it's not really possible, but I was wondering if there's a really better/clearer way to do this.
I see that you want to say here that it's fine if VPR is the last operand, because that means it's predicated, and it is a 'real' use if it is not a def and not the last operand..

Can you perhaps simply not iterate over the last one:

  i < MI->getNumOperands() - 1

and then simply have:

  if (MO.isDef())
      CurrentPredicate.insert(MI);
  else if (MO.isUse())
      ...


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

https://reviews.llvm.org/D71107