[PATCH] D75033: [MachineVerifier] Doing ::calcRegsPassed over faster sets: ~15-20% faster MV

[Mark Lacey via Phabricator via llvm-commits]

rudkx accepted this revision.
rudkx added a comment.
This revision is now accepted and ready to land.

LGTM.



================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:2131
+    for (unsigned Reg : FromRegSet)
+      if (Register::isVirtualRegister(Reg)) {
+        unsigned Index = Register::virtReg2Index(Reg);
----------------
rtereshin wrote:
> rudkx wrote:
> > You could reduce indentation here by adding braces around the body of the `for` loop, inverting the condition, and using `continue`, per https://www.llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code.
> I could indeed! The body has started as one-liner, then grew, and I haven't changed this.
> 
> I'll update it that way in one go with the rest of the nitpicks when those are all available together, and if the patch clears overall. Thanks!
SGTM!


================
Comment at: llvm/lib/CodeGen/MachineVerifier.cpp:2190
+public:
+  // Set-up the FILTERb. \pre Input register set \p RS must have no duplicates.
+  // Both virtual and physical registers are fine.
----------------
rtereshin wrote:
> rtereshin wrote:
> > rudkx wrote:
> > > FILTERb seems like a typo.
> > Not a typo, intended is `OUT_b`, as a subscript, for "block". FILTER is unique per BB.
> Does that clears the issue, or you'd rather prefer those subscripts dropped (I suppose everywhere then, not just on `FILTERb`)?
> 
> I don't have a preference, fine either way.
I hadn't noticed the comment before the `class` which is why it stuck out. I guess I'm not opposed to any particular convention as long as we're consistent here but the all-caps name with lower-case suffix did stick out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75033