[PATCH] D82197: [NFCI] Cleanup range checks in Register/MCRegister

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/Register.h:72
     assert(!isStackSlot(Reg) && "Not a register! Check isStackSlot() first.");
-    return int(Reg) < 0;
+    return Reg >= MCRegister::FirstVirtualReg;
   }
----------------
Note: We use the number interpretation here but then the bitmask interpretation below. Please see later comment.


================
Comment at: llvm/include/llvm/CodeGen/Register.h:79
     assert(isVirtualRegister(Reg) && "Not a virtual register");
-    return Reg & ~(1u << 31);
+    return Reg & ~MCRegister::FirstVirtualReg;
   }
----------------
I think we should stick purely with either the bitmask or the number interpretation. The name of `MCRegister::FirstVirtualReg` does not work well with the bitmask interpretation; however, since the bitmask operation is only valid given information that is not readily apparent to the compiler here (without adding a cast to `uint32_t` and changing the `assert` to `llvm_unreachable`), I guess the answer is to rename in favour of the bitmask interpretation. I'm using `VirtualRegFlag` in other comments for discussion purposes.


================
Comment at: llvm/include/llvm/CodeGen/Register.h:85
   static unsigned index2VirtReg(unsigned Index) {
-    return Index | (1u << 31);
+    return Index | MCRegister::FirstVirtualReg;
   }
----------------
Should there be an assert that the "index" does not already have the indicator bit set?


================
Comment at: llvm/include/llvm/MC/MCRegister.h:39
+  static_assert(sizeof(Reg) >= 4, "Reg isn't large enough to hold full range.");
+  static constexpr unsigned NoRegister = 0u;
+  static constexpr unsigned FirstPhysicalReg = 1u;
----------------
Would `NullValue` be correct?


================
Comment at: llvm/include/llvm/MC/MCRegister.h:52
   static bool isStackSlot(unsigned Reg) {
-    return int(Reg) >= (1 << 30);
+    return (Reg & FirstStackSlot) && Reg < FirstVirtualReg;
   }
----------------
To be consistent between using mask versus arithmetic:
```
!(Reg & VirtualRegFlag) && uint32_t(Reg) >= FirstSlackSlot;
```
if you want better code from GCC or
```
!(Reg & VirtualRegFlag) && uint32_t(Reg & ~VirtualRegFlag) >= FirstSlackSlot;
```
if you want better code from Clang.



================
Comment at: llvm/include/llvm/MC/MCRegister.h:59
     assert(!isStackSlot(Reg) && "Not a register! Check isStackSlot() first.");
-    return int(Reg) > 0;
+    return Reg >= FirstPhysicalReg && Reg < FirstVirtualReg;
   }
----------------
Note: Number (and not bitmask) interpretation here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82197