[PATCH] D75452: [ARM][MVE] Validate tail predication values

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:555
+  // be true, the newly inserted implicit predication must not change the
+  // the results. All MVE loads and stores have to be predicated, so we know
+  // that any load operands, or stored results are equivalent already.
----------------
Would it be good to spell out here that we have these 2 use cases?
- IR produced by the vectoriser, and
- IR originating from hand written (vector) intrinsics, which can also trigger tail-predication.

loads/stores produced by the vectoriser (when it thinks tail-folding is good) will all be nicely masked, so this concerns the 2nd use case. Is that right?



================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:560
+  // preventing many instructions from updating their falsely predicated
+  // lanes. This analyse assumes that all the instructions perform lane-wise
+  // operations and don't perform any exchanges.
----------------
nit: analyse -> analysis?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:574
+  SetVector<MachineInstr *> UnknownFalseLanes;
+  SmallPtrSet<MachineInstr *, 4> KnownFalseZeros;
+  for (auto &MI : *MBB) {
----------------
I appreciate this is bikeshedding, but here we go anyway: UnknownFalseLanes and KnownFalseZeros is a bit difficult to read (in the code below), but your descriptions/comments above are clear. Summarising that, we have a set of instructions with some behaviour that we (immediately) understand, and there are some instructions that needs to be further analysed. 
So, was wondering `KnownFalseZeros` could be named something along the lines of just `Predicated`, and `UnknownFalseLanes` be the `Worklist` of things to check?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:589
+    for (auto &MO : MI.operands()) {
+      if (!isRegInClass(MO, QPRs) || !MO.isUse() || MO.getReg() != DefReg)
+        continue;
----------------
can you use one of your new RDA helper functions for this?


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

https://reviews.llvm.org/D75452