[PATCH] D87610: [ARM] Fix tail predication predicate tracking

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added a comment.

I understood there is a NFC and non-NFC part of this patch. Is worth separating this out?



================
Comment at: llvm/lib/Target/ARM/ARMInstrVFP.td:2849
+let Predicates = [HasV8_1MMainline, HasMVEInt],
+    D = MVEDomain, validForTailPredication=1 in {
   let Uses = [VPR] in {
----------------
High-level question: is this worth to create a separate patch for this? Because we are adding MVEDomain, is this easily tested with assembly/disassembly tests?


================
Comment at: llvm/lib/Target/ARM/ARMInstrVFP.td:2867
 
-let Predicates = [HasV8_1MMainline, HasMVEInt] in {
+let Predicates = [HasV8_1MMainline, HasMVEInt],
+    D = MVEDomain, validForTailPredication=1 in {
----------------
This is redundant and we can just use the predicate on line 2848?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:857
+//   is in a tail predicated loop.
+// This means that predicated VPR defs build up an AND chain of sequential
+// predicates. When we perform the tail predication conversion, we effectively
----------------
Can you clarify here if the "this means"  applies more to the second point above, or to both?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:859
+// predicates. When we perform the tail predication conversion, we effectively
+// have a loop contained within a large VPT, where the VPR is reset for each
+// loop iteration and the rest of the defs for the iteration build up by
----------------
a large VPT -> a large VPT block? 


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:862
+// ANDing their result with the current VPR.P0 value. So, we need to figure out
+// whether each the predication will be equivalent in the current loop and in
+// its tail predicated form:
----------------
each the predication -> each predicate?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:886
 
-  if (isVCTP(MI)) {
-    // If we find another VCTP, check whether it uses the same value as the main VCTP.
-    // If it does, store it in the SecondaryVCTPs set, else refuse it.
-    if (VCTP) {
-      if (!VCTP->getOperand(1).isIdenticalTo(MI->getOperand(1)) ||
-          !RDA.hasSameReachingDef(VCTP, MI, MI->getOperand(1).getReg())) {
-        LLVM_DEBUG(dbgs() << "ARM Loops: Found VCTP with a different reaching "
-                             "definition from the main VCTP");
-        return false;
-      }
-      LLVM_DEBUG(dbgs() << "ARM Loops: Found secondary VCTP: " << *MI);
-      SecondaryVCTPs.insert(MI);
-    } else {
-      LLVM_DEBUG(dbgs() << "ARM Loops: Found 'main' VCTP: " << *MI);
-      VCTP = MI;
-    }
-  } else if (isVPTOpcode(MI->getOpcode())) {
+  // Don't check non-mve instructions.
+  const MCInstrDesc &MCID = MI->getDesc();
----------------
mve -> MVE


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:904
+
+  // Beginning a new VPT block.
+  if (isVPTOpcode(MI->getOpcode())) {
----------------
Beginning -> Beginning of?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:915
     CurrentBlock = &VPTBlocks.back();
+    LLVM_DEBUG(dbgs() << "ARM Loops: Created new VPT block with predicate:\n";
+               for (auto *PI : CurrentPredicate)
----------------
Perhaps just a nit about the message, but we are not really creating new VPT blocks here?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:927
   bool IsUse = false;
-  bool IsDef = false;
-  const MCInstrDesc &MCID = MI->getDesc();
+  bool IsDef = MI->findRegisterDefOperandIdx(ARM::VPR) != -1;
   for (int i = MI->getNumOperands() - 1; i >= 0; --i) {
----------------
Move this where it is used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D87610