[PATCH] D72509: [ARM][LowOverheadLoops] Allow all MVE instrs.

[Sam Parker via Phabricator via llvm-commits]

samparker marked 3 inline comments as done.
samparker added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:525
+  if ((Flags & ARMII::DomainMask) != ARMII::DomainMVE)
+    return true;
+
----------------
dmgreen wrote:
> What if we had something like a `VMOV s0 s1`. I know that's not anything to do with this patch, but is that outlawed anywhere at the moment?
It's not outlawed., why do we need to be concerned about it?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:530
+  // VPT block.
+  if ((Flags & ARMII::ValidForTailPredication) == 0 && !IsUse) {
+    LLVM_DEBUG(dbgs() << "ARM Loops: Can't tail predicate: " << *MI);
----------------
dmgreen wrote:
> What about VPNOT or VPSEL? They would use both the vpr and the tail predicate to different extents.
> 
> VPSEL we seem to mark as IsValidForTailPredication, which I'm not sure about. We need to make sure it's using the "same" predicate as before (and making sure it doesn't just use whatever we had last put there, if we delete the vcpt!)
> 
> 
Good point. Yes, it looks like the problem is that we're only recording VPT blocks whereas, as you say, there are other instructions that also need handling. It looks like VPSEL/VPNOT would currently stuffed into the previous VPT block, even if they're not in one. This will mean that the predicates are checked for correctness, but it also could cause an assert too! I'll need to put up a different patch for that change.


================
Comment at: llvm/test/CodeGen/Thumb2/LowOverheadLoops/vmldava_in_vpt.mir:174
+  ; CHECK:   renamable $q3 = MVE_VMINu32 renamable $q2, renamable $q0, 0, $noreg, undef renamable $q3
+  ; CHECK:   renamable $r12 = MVE_VMLADAVas32 killed renamable $r12, killed renamable $q3, killed renamable $q2, 0, killed $noreg
+  ; CHECK:   $lr = MVE_LETP killed renamable $lr, %bb.2
----------------
dmgreen wrote:
> Another unrelated one, but there is nothing in the instructions of this loop that says that this is predicated on a hardware register, right? Or that is has unmodelled side effects? The refs to vpr and the vctp are removed but no other ref is added.
Yes, at this point it's completely implicit, which I don't think is a problem currently. Maybe we could add a new register and a predicate def for LSTP and LETP, but I have no idea how invasive that would be for the predicate users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72509