This is an archive of the discontinued LLVM Phabricator instance.

Switch magic numbers to library functions in fixup
ClosedPublic

Authored by dhoekwater on Jun 13 2023, 11:07 AM.

Details

Summary

Using masks and bounds that are magic numbers
means that defects in bounds checking and offset
masking are subtle and hard to detect.
https://reviews.llvm.org/D152841 is an example
of this type of defect. Switching to clearly
readable library functions makes defects less
obfuscated.

Diff Detail

Event Timeline

dhoekwater created this revision.Jun 13 2023, 11:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 11:07 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
dhoekwater requested review of this revision.Jun 13 2023, 11:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 13 2023, 11:07 AM

I like using isInt() and isUInt() more.

isShiftedUInt() also checks the low bits, so I think this changes the diagnostic output in some cases?

I don't like Value != alignTo<4>(Value); alignTo<4>(Value) doesn't represent any actual relevant value.

Not sure about replacing 0x3ffffff with maskTrailingOnes<uint64_t>(26); it's really verbose, and the name isn't great. Maybe if we can come up with some shorter alternative? Or maybe just write ((1ULL << 26) - 1)?

I have similar opinions to efriedma. Definitely like using isUint and isInt more. I'd prefer to use isAligned rather than alignTo.

I'm so used to seeing trailing masks, I do find maskTrailingOnes less intuitive, but will go with consensus on that front. Perhaps make a simpler/shorter name with a using?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
175

While I think they will give the same result as this is a no-op for an already 4-byte aligned quantity it is a more extensive calculation as alignTo aligns to the next 4-byte boundary.

Would isAligned(Value, 4) in Support/Alignment.h be better?

dhoekwater updated this revision to Diff 532343.EditedJun 16 2023, 6:04 PM

Fixed the isShiftedUInt() issue that changed behavior.

Replaced alignTo<4> stuff with isAligned(Align{4}, x). That's a neat utility function I didn't know existed.

I also don't love how verbose maskTrailingOnes<uint64_t>(n) is. I do personally feel like anything that
directly indicates the number of bits we're taking is better than a magic number mask, but I also haven't
developed the trained eye that tells me "0x1FFFFF is 21 bits" at a glance. Is there a more terse helper
function to express "get the low n bits of this number"? Ideally, we'd have something that clearly communicates
the meaning of something like n & 0x3, (n & 0x1ffffc) >> 2, or (n & 0x1fffff000ULL) >> 12. If not,
I'll just revert the masking changes.

Rebase onto main and undo masking changes. This *should* be the final push.

dhoekwater edited the summary of this revision. (Show Details)Jun 22 2023, 12:15 PM
dhoekwater marked an inline comment as done.
MaskRay added inline comments.Jun 22 2023, 5:09 PM
llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
175

For this case, the original Value & 0x3 seems clearer to me than the new !isAligned(Align{4}, Value)

dhoekwater added inline comments.Jun 22 2023, 5:40 PM
llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
175

I think using a library function here is more clear, but maybe that's because the average reader of this code is more comfortable with visualizing bitmasks in their head than I am.

This kind of comes back to the "is it clearer to have the reader visualize hex values in their head or have it spelled out?" conversation regarding masks. In this case, the intent is to convey that we are checking if Value is 4-byte aligned.

The original version is shorter, but it has more logical steps to arrive at the semantic meaning ("we're checking if Value is aligned to a 4-byte boundary") than the new version.

arsenm accepted this revision.Jul 26 2023, 1:29 PM
arsenm added inline comments.
llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
202

I'm mostly surprised by the parameter ordering of isAligned, I'd expect the alignment value to be the second argument

This revision is now accepted and ready to land.Jul 26 2023, 1:29 PM
MaskRay added inline comments.Jul 26 2023, 2:59 PM
llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp
202

I am still of the opinion that isAligned(Align{4}, Value)) is a very awkward way to test alignment. We don't do this in other places, and I don't think it's a good suggestion.

isUInt is fine.

This revision was automatically updated to reflect the committed changes.
dhoekwater marked 3 inline comments as done.