[PATCH] D87681: [ARM] Improve VPT predicate tracking

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:165
+    static SmallVector<VPTBlock, 4> Blocks;
+    static SetVector<MachineInstr *> CurrentPredicate;
+    static std::map<MachineInstr *,
----------------
In one of the other patches, you had a nice comment explaining the chaining and AND'ing of predicates. Is that applicable here, would that comment be good to have here? 


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:155
 
   // Represent a VPT block, a list of instructions that begins with a VPT/VPST
   // and has a maximum of four proceeding instructions. All instructions within
----------------
Is this comment out of date?  This class is not just representing a VPT block anymore as it e.g. contains member `Blocks`? A real proper nit in that case: perhaps VPTState or VPBlocks or something similar is a more accurate description of this class 


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:162
 
-  public:
-    VPTBlock(MachineInstr *MI, SetVector<MachineInstr*> &Preds) {
-      PredicateThen = std::make_unique<PredicatedMI>(MI, Preds);
+    SmallVector<MachineInstr *, 4> Insts;
+
----------------
Can you add a comment what this is? For example, at a first glance I was curious what the difference was with PredicatedInsts.


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:165
+    static SmallVector<VPTBlock, 4> Blocks;
+    static SetVector<MachineInstr *> CurrentPredicate;
+    static std::map<MachineInstr *,
----------------
In one of the other patches, you had a nice comment explaining the chaining and AND'ing of predicates. Would it be good/applicable to have that comment  here?
A proper nit in that case: CurrentPredicate -> CurrentPredicates?


================
Comment at: llvm/lib/Target/ARM/ARMLowOverheadLoops.cpp:1371
+        LoLoop.ToRemove.insert(Insts.front());
+        for (unsigned i = 1; i < Insts.size(); ++i)
+          RemovePredicate(Insts[i]);
----------------
just checking, should this be `<=`?


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

https://reviews.llvm.org/D87681