This is an archive of the discontinued LLVM Phabricator instance.

[NFCI] Cleanup range checks in Register/MCRegister
ClosedPublic

Authored by daltenty on Jun 19 2020, 8:07 AM.

Details

Summary

by removing casts from unsigned to int that which may be implementation
defined according to C++14 (and thus trip up the XL compiler on AIX) by
just using unsigned comparisons/masks and refactor out the range
constants to cleanup things a bit while we are at it.

Diff Detail

Event Timeline

daltenty created this revision.Jun 19 2020, 8:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 19 2020, 8:07 AM
daltenty retitled this revision from Revert "[FileCheck, unittest] Improve readability of ExpressionFormat" to [NFCI] Cleanup range checks in Register/MCRegister.Jun 19 2020, 8:09 AM
daltenty edited the summary of this revision. (Show Details)
llvm/include/llvm/CodeGen/Register.h
36

Please use numeric_limits

  • to catch if the type of Reg changes to in a way that the size no longer accurately reflects the range, and
  • account for padding bits.
llvm/include/llvm/MC/MCRegister.h
38

Same comment.

llvm/include/llvm/CodeGen/Register.h
72

Note: We use the number interpretation here but then the bitmask interpretation below. Please see later comment.

79

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.

85

Should there be an assert that the "index" does not already have the indicator bit set?

llvm/include/llvm/MC/MCRegister.h
39

Would NullValue be correct?

52

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.

59

Note: Number (and not bitmask) interpretation here.

daltenty marked an inline comment as done.Jun 22 2020, 7:43 AM
daltenty added inline comments.
llvm/include/llvm/MC/MCRegister.h
39

Used NoRegister to be consistent with what TableGen calls this value.

daltenty marked 6 inline comments as done.Jun 22 2020, 5:43 PM
daltenty updated this revision to Diff 272578.Jun 22 2020, 5:45 PM
  • Address comments round 1
    • Use numeric_limits for static assert
    • Use bit mask interpretation for VirtualRegFlag
    • Add range assert for index to virtual reg conversion
This revision is now accepted and ready to land.Jun 22 2020, 6:10 PM
This revision was automatically updated to reflect the committed changes.