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

[Pierre van Houtryve via Phabricator via llvm-commits]

Pierre-vh added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:170
+    // instruction that performs a comparison.
+    bool isVPT() const { return !isVPT(); }
+
----------------
samparker wrote:
> Something's a miss here! This should have been caught by a test.
This function was unused, which is why the mistake was not caught in a test - I fixed it now.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:803
+    if (VCTP) {
+      if (!VCTP->getOperand(1).isIdenticalTo(MI->getOperand(1)))
+        return false;
----------------
samparker wrote:
> isIdentical is not going to do what you're hoping for here. Use RDA is figure out if both VCTPs are operating on the same value (not just the same register). Also needs a test. I'm wondering whether we should also assert that the VCTP, if predicated, is 'Then' predicated. An example you had (last week?) confused me because I'm not sure why an Else predicate VCTP should appear here and how it maps the current idea that all predicates are ANDed. 
>  Also needs a test. I'm wondering whether we should also assert that the VCTP, if predicated, is 'Then' predicated. An example you had (last week?) confused me because I'm not sure why an Else predicate VCTP should appear here and how it maps the current idea that all predicates are ANDed.

I found an example where a else-predicated VCTP is generated and I added it to the tests, it comes from this C++ code:
```
void test(int * data, int N, int T)
{
  for(int i = 0; i < N; i++) {
    int d = data[i];
    if (d < T || d > -T)
      data[i] = 0;
  }
}
```


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:811
+    }
+  } else if (isVPTOpcode(MI->getOpcode())) {
+    // Start a new vpt block when we discover a VPT or a VPST.
----------------
samparker wrote:
> Following on from my above comment... the logic here was assuming that each value in VPR.P0 is rooted at the VCTP and then subsequent instructions (only VCMP) can modify the VPR and those instructions have to predicated on the VCTP. This should result in a predicate along the lines of VCTP && VCMP.  Now we're trying to allow VPTs, which create a predicate and cannot be predicated by an existing VPT block - so how do we know that the resulting predicate is still rooted at VCTP and not 'disjoint'? 
> Everything in this pass assumes that any instruction using VPR is predicated on the VCTP and this is the place where we need to make that guarantee. So a couple of things that I can think of now are:
> - that the isVectorPredicated function actually means 'isPredicatedOnVCTP' and that will no longer be true.
> - the notion of 'disjoint' has become more complicated, though a VPT doesn't use the VPR value defined by VCTP, it could be using arguments that are dependent upon the VCTP - and I have the feeling this is the only case that we can support.
> 
> These nuances will also require more testing.
> the notion of 'disjoint' has become more complicated, though a VPT doesn't use the VPR value defined by VCTP, it could be using arguments that are dependent upon the VCTP - and I have the feeling this is the only case that we can support.

I added a check in `ValidateMVEInst` - It'll now refuse VPTs if none of their operands is defined by a predicated instruction, and I added a test for this. Is this enough?


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

https://reviews.llvm.org/D78206