[PATCH] D82962: [Alignment][NFC] Use Align for BPFAbstractMemberAccess::RecordAlignment
Guillaume Chatelet via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jul 1 09:10:30 PDT 2020
gchatelet marked an inline comment as done.
gchatelet added inline comments.
================
Comment at: llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp:120
uint32_t AccessIndex;
- uint32_t RecordAlignment;
+ Align RecordAlignment;
MDNode *Metadata;
----------------
courbet wrote:
> I cannot convince myself that this is an NFC. Aggregate initialization will initialize `RecordAlignment` to `0`, and there are some cases when `RecordAlignment` is not set by code (e.g. `BPFAbstractMemberAccess::IsPreserveDIAccessIndexCall`).
>
> On the other hand, the only usage is
> ```
> uint32_t MemberBitSize = MemberTy->getSizeInBits();
> uint32_t MemberBitOffset = MemberTy->getOffsetInBits();
> uint32_t AlignBits = RecordAlignment * 8;
> if (RecordAlignment > 8 || MemberBitSize > AlignBits)
> report_fatal_error("Unsupported field expression for llvm.bpf.preserve.field.info, "
> "requiring too big alignment");
> ```
>
> So it looks like `0` would always lead to a fatal error anyway, so non-zero is really not intended.
I find this puzzling because the error specifically mentions "llvm.bpf.preserve.field.info" and if you look at `IsPreserveDIAccessIndexCall`, the only case where `RecordAlignment` can be `0` is when `GV->getName().startswith("llvm.bpf.preserve.field.info")`. On all other cases `RecordAlignment>0` and `GV->getName()` would start with one of:
- "llvm.preserve.array.access.index"
- "llvm.preserve.union.access.index"
- "llvm.preserve.struct.access.index"
So it doesn't make sense to me.
As you said, `0` would lead to a fatal error so I take this as `RecordAlignment!=0`.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82962/new/
https://reviews.llvm.org/D82962
More information about the llvm-commits
mailing list