[PATCH] D76709: [Target][ARM] Adding MVE VPT Optimisation Pass

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/Target/ARM/MVEVPTOptimisationsPass.cpp:141
+MachineInstrBuilder
+MVEVPTOptimisations::BuildVPNOTBefore(MachineBasicBlock &MBB,
+                                      MachineInstr &Instr) {
----------------
How much does this function add? It doesn't seem to do a huge amount.


================
Comment at: llvm/lib/Target/ARM/MVEVPTOptimisationsPass.cpp:158-159
+    // Ignore predicated instructions.
+    if (getVPTInstrPredicate(Instr) != ARMVCC::None)
+      continue;
+
----------------
Do you have any tests for this bit?


================
Comment at: llvm/lib/Target/ARM/MVEVPTOptimisationsPass.cpp:179-180
+    // Build a VPNOT to replace the VCMP, reusing its operands.
+    MachineInstrBuilder MIBuilder = BuildVPNOTBefore(MBB, Instr);
+    MIBuilder.add(Instr.getOperand(0));
+    MIBuilder.addReg(PrevVCMPResultReg);
----------------
You can do BuildMI(...).add(Instr.getOperand(0)).addReg(PrevVCMPResultReg)...


================
Comment at: llvm/lib/Target/ARM/MVEVPTOptimisationsPass.cpp:182-183
+    MIBuilder.addReg(PrevVCMPResultReg);
+    MIBuilder.add(Instr.getOperand(4));
+    MIBuilder.add(Instr.getOperand(5));
+    LLVM_DEBUG(dbgs() << "  Inserting VPNOT (to replace VCMP): ";
----------------
Operand 4 and 5 are always None and noreg? If so you can use addUnpredicatedMveVpredNOp, which makes it more obvious what the operands are expected to be.


================
Comment at: llvm/test/CodeGen/Thumb2/mve-vpt-optimisations.mir:154-155
+    liveins: $q0, $q1
+    renamable $vpr = MVE_VCMPf16 renamable $q0, renamable $q1, 10, 0, $noreg
+    renamable $vpr = MVE_VCMPf16 renamable $q0, renamable $q1, 11, 0, $noreg
+
----------------
I was expecting these registers to be virtual, given where this is in the pipeline. Will they be physical instead?


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

https://reviews.llvm.org/D76709