[PATCH] D71470: Recommit "[ARM][MVE] findVCMPToFoldIntoVPS"

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

Hello. I had to revert this in https://reviews.llvm.org/rGd50e188a072deca9d48149e05a05756c474bf569 because it was causing some problems to do with checking for uses between the VCMP and the VPT block it is folded into. When looking I didn't think that some of the other details here looked right either.

I've tried to keep your tests as they still seemed useful, and will add a new one for the case I was looking at today.

Sorry for not really looking at it earlier. I wasn't paying enough attention after the first revision. I have put some extra comments below.



================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:146
-    --CmpMI;
-    if (CmpMI->modifiesRegister(ARM::VPR, TRI))
-      break;
----------------
What happened to this modifies register check?

This was the original error I was looking at.


================
Comment at: llvm/lib/Target/ARM/MVEVPTBlockPass.cpp:254
 bool MVEVPTBlock::runOnMachineFunction(MachineFunction &Fn) {
+  if (skipFunction(Fn.getFunction()))
+    return false;
----------------
I don't think it's valid to skip this at -O0. It is needed for valid assembly.

You could argue that some of the optimisations this pass is doing shouldn't be done at O0, but at least VPST's need to be added.


================
Comment at: llvm/test/CodeGen/ARM/O3-pipeline.ll:147
 ; CHECK-NEXT:      If Converter
+; CHECK-NEXT:      ReachingDefAnalysis
 ; CHECK-NEXT:      MVE VPT block insertion pass
----------------
RDA looks like quite an expensive pass to me. It would scan through and store reaching defs info for every instruction in every block of a function?

Do you think it's worth it for this case, where we are only doing some minor code cleanup?


================
Comment at: llvm/test/CodeGen/Thumb2/mve-vpt-block-optnone.mir:72
+      $vpr = VMSR_P0 killed $r0, 14, $noreg
+    renamable $q0 = nnan ninf nsz MVE_VMINNMf32 killed renamable $q1, killed renamable $q2, 1, killed renamable $vpr, killed renamable $q0
+    tBX_RET 14, $noreg, implicit $q0
----------------
If this instruction is predicated, then we need to generate a VPST, otherwise the final assembly will be invalid (the "Then" predicate will be silently dropped as this is not part of a valid VPT block).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71470