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

[Sam Parker via Phabricator via llvm-commits]

samparker marked 3 inline comments as done.
samparker 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.
----------------
SjoerdMeijer wrote:
> 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?
> 
This pass shouldn't be concerned with whether the input is from the vectorizer or the intrinsics, as we can't know. On line 762 we enforce that all MVE memory operations are explicitly predicated.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:574
+  SetVector<MachineInstr *> UnknownFalseLanes;
+  SmallPtrSet<MachineInstr *, 4> KnownFalseZeros;
+  for (auto &MI : *MBB) {
----------------
SjoerdMeijer wrote:
> 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?
I did have Predicated at some point, but I thought that was misleading because some instructions won't be using the VPR. I like KnownFalseZeros because it's explicit in what we expect and I'm much rather have code that doesn't require a big comment to make any sense of what is going on. It doesn't have to be that, but its name should convey what's important about the set. The naming of UnknownFalseLanes is definitely less important though.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:589
+    for (auto &MO : MI.operands()) {
+      if (!isRegInClass(MO, QPRs) || !MO.isUse() || MO.getReg() != DefReg)
+        continue;
----------------
SjoerdMeijer wrote:
> can you use one of your new RDA helper functions for this?
There were just static helpers and I don't think we won't really do enough of this logic to warrant more helpers.


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

https://reviews.llvm.org/D75452