[PATCH] D76847: [Target][ARM] Replace re-uses of old VPR values with VPNOTs

[Pierre van Houtryve via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/ARM/MVEVPTOptimisationsPass.cpp:240
+
+  Register VCCRValue, OppositeVCCRValue;
+  // The first loop looks for 2 unpredicated instructions:
----------------
dmgreen wrote:
> Does this only find the first pair in a basic block? What would happen if there are multiple (potential) vpt blocks in a larger basic block, and several of them can be optimized?
> 
> Can this be done with a linear scan through the block that attempts to find and hold VPNOT's and the original value, converting any uses of the original to the VPNOT. Or does that not work very well for some reason? Maybe using something like MRI->use_instructions would also help.
> What would happen if there are multiple (potential) vpt blocks in a larger basic block, and several of them can be optimized?

Unfortunately it'd only optimize the first one as it'd only pick up the first pair of VCMP/VPNOT and not the second.
I can try to rewrite the function so it handles those cases, I'll do that tomorrow.

> Can this be done with a linear scan through the block that attempts to find and hold VPNOT's and the original value, converting any uses of the original to the VPNOT. Or does that not work very well for some reason?

I don't think it'd work well, but I can certainly try it and see if it works. I'll also look at `use_instructions`, but I'm not sure it'll help.



================
Comment at: llvm/lib/Target/ARM/MVEVPTOptimisationsPass.cpp:256
+      // no users.
+      if (!MoveVPNOTBeforeFirstUser(MBB, Iter, VCCRValue))
+        continue;
----------------
dmgreen wrote:
> Can you explain the advantage of moving the VPNOT?
Sometimes, you can have code like this (taken from the bottom test):

```
    %2:vccr = MVE_VCMPs32 %0:mqpr, %1:mqpr, 10, 0, $noreg
    %3:vccr = MVE_VPNOT %2:vccr, 0, $noreg
    %4:mqpr = MVE_VORR %0:mqpr, %1:mqpr, 1, %2:vccr, undef %4:mqpr
    %5:mqpr = MVE_VORR %1:mqpr, %0:mqpr, 1, %2:vccr, undef %5:mqpr
    %6:mqpr = MVE_VORR %4:mqpr, %5:mqpr, 1, %3:vccr, undef %6:mqpr
```

`%3` is not used directly: the original VCCR value, `%2` is used before it, so their lifetimes overlap.

If I didn't move the VPNOT further down in such situations, the pass would insert a double VPNOT,  like this:
```
    %2:vccr = MVE_VCMPs32 %0:mqpr, %1:mqpr, 10, 0, $noreg
    %3:vccr = MVE_VPNOT %2:vccr, 0, $noreg
    %foo:vccr = MVE_VPNOT %3:vccr
    %4:mqpr = MVE_VORR %0:mqpr, %1:mqpr, 1, %foo:vccr, undef %4:mqpr
    %5:mqpr = MVE_VORR %1:mqpr, %0:mqpr, 1, %foo:vccr, undef %5:mqpr
    %bar:vccr = MVE_VPNOT %foo:vccr
    %6:mqpr = MVE_VORR %4:mqpr, %5:mqpr, 1, %bar:vccr, undef %6:mqpr
``` 

But, since I now move the VPNOT further down, we get this instead, which is of course much better.

```
    // VPNOT moved down, no more overlapping VCCR lifetimes, no double VPNOTs.
    %2:vccr = MVE_VCMPs32 %0:mqpr, %1:mqpr, 10, 0, $noreg
    %4:mqpr = MVE_VORR %0:mqpr, %1:mqpr, 1, %2:vccr, undef %4:mqpr
    %5:mqpr = MVE_VORR %1:mqpr, %0:mqpr, 1, %2:vccr, undef %5:mqpr
    %3:vccr = MVE_VPNOT %2:vccr, 0, $noreg
    %6:mqpr = MVE_VORR %4:mqpr, %5:mqpr, 1, %3:vccr, undef %6:mqpr
```

That transformation is of course only done if a use of the orginal VCCR value is found before the first use of the VPNOT's result.


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

https://reviews.llvm.org/D76847