[PATCH] D78206: [Target][ARM] Make Low Overhead Loops coexist with VPT blocks

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh marked 2 inline comments as done.
Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:230
+      MachineInstr *Op2Def = RDA.getMIOperand(MI, MI->getOperand(2));
+      if(Op1Def == nullptr && Op2Def == nullptr)
+        return false;
----------------
samparker wrote:
> I'm still pondering about this... I think this should be checking that both operands are either predicated q-regs, scalar or are loop invariant, that way should ensure that the result will be the same after the transform. This method also restricts us to instructions that are directly predicated, so it would be good to add a TODO here as well.
> I'm still pondering about this... I think this should be checking that both operands are either predicated q-regs, scalar or are loop invariant, that way should ensure that the result will be the same after the transform.

If I understand correctly, this function currently only handles the first case (predicated q-regs), but it should be expanded to allow any r-reg and loop invariants (registers that aren't defined inside any of the loop's basic blocks) ?

> This method also restricts us to instructions that are directly predicated, so it would be good to add a TODO here as well.

What do you mean by "directly predicated"? Can you please give me an example of an indirectly predicated instruction in this context?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1239
+  //    - b. When the block uses a vpst, and is only predicated on the vctp
+  //    - c. When the block uses a vpt and contains one or more vctp
+  // 2. VPT Blocks with uniform predicates:
----------------
samparker wrote:
> Does the block have to contain a vctp? Do we check that this is true?
It doesn't have to contain a VCTP. I'll change this comment to make it clear.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1299
+      }
+    } else if (Block.IsOnlyPredicatedOn(LoLoop.VCTP) && Block.isVPST()) {
+      // A vpt block starting with VPST, is only predicated upon vctp and has no
----------------
samparker wrote:
> I might be viewing the nesting incorrectly here, but isn't this handled on line 1250?
The if at line 1250 is inside the if at line 1244. That else-if is from the if at line 1244.
  - if (1244)
     - if (1250)
  - else if (1299)




================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1315
+  // Remove remaining secondary VCTPs
+  for (MachineInstr *VCTP : LoLoop.SecondaryVCTPs) {
+    // All VCTPs that aren't marked for removal yet should be unpredicated ones.
----------------
samparker wrote:
> Surely we should have discovered all of these during the loop above?
The loop above catches the VCTP inside VPT blocks, but not the other ones.

So for instance, if you have this (from `/test/CodeGen/Thumb2/LowOverheadLoops/vctp-in-vpt-2.mir`)

```
    MVE_VPST 4, implicit $vpr
    renamable $vpr = MVE_VCTP32 renamable $r2, 1, killed renamable $vpr
    renamable $r1, renamable $q0 = MVE_VLDRWU32_post killed renamable $r1, 16, 1, killed renamable $vpr :: (load 16 from %ir.lsr.iv24, align 4)
    renamable $vpr = MVE_VCTP32 renamable $r2, 0, $noreg
    renamable $r2, dead $cpsr = tSUBi8 killed renamable $r2, 4, 14, $noreg
```

The first VCTP will be removed by the loop above, but the "secondary" one below won't - it'll be removed by this loop.


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

https://reviews.llvm.org/D78206