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

[Sam Parker via Phabricator via llvm-commits]

samparker added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:170
+    // instruction that performs a comparison.
+    bool isVPT() const { return !isVPT(); }
+
----------------
Something's a miss here! This should have been caught by a test.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:803
+    if (VCTP) {
+      if (!VCTP->getOperand(1).isIdenticalTo(MI->getOperand(1)))
+        return false;
----------------
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. 


================
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.
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:825
   //    instruction, such as vcmp, that can provide the VPR def.
+  else if (MI->getOpcode() == ARM::MVE_VPSEL ||
+           MI->getOpcode() == ARM::MVE_VPNOT)
----------------
Format this differently so this statement is attached to the rest of the conditional blocks.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1214
   // 4) A VPT block which is not predicated upon a vctp, but contains it and
   //    all instructions within the block are predicated upon in.
 
----------------
Document the new case(s).


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

https://reviews.llvm.org/D78206