[PATCH] D152843: Switch magic numbers to library functions in fixup

[Daniel Hoekwater via Phabricator via llvm-commits]

dhoekwater added inline comments.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64AsmBackend.cpp:175
       Ctx.reportError(Fixup.getLoc(), "fixup value out of range");
-    if (Value & 0x3)
+    if (Value != alignTo<4>(Value))
       Ctx.reportError(Fixup.getLoc(), "fixup not sufficiently aligned");
----------------
MaskRay wrote:
> peter.smith wrote:
> > 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?
> > 
> > 
> > 
> For this case, the original `Value & 0x3` seems clearer to me than the new `!isAligned(Align{4}, Value)`
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152843