This is an archive of the discontinued LLVM Phabricator instance.

[LangRef] Require i8s to be naturally aligned
ClosedPublic

Authored by jsilvanus on Jan 20 2023, 6:56 AM.

Details

Summary

It is widely assumed that i8 is naturally aligned (i8:8),
and that hence i8s can be used to access arbitrary bytes.

As discussed in https://discourse.llvm.org/t/status-of-overaligned-i8,
this patch makes this assumption explicit, by documenting it in
the LangRef, and enforcing it when parsing a data layout string.

Historically, there have been data layouts that violate this requirement,
notably the old DXIL data layout that aligns i8 to 32 bits.

A previous patch (df1a74a) enabled importing modules with invalid data layouts
using override callbacks.
Users who wish to continue importing modules with overaligned i8s (e.g. DXIL)
thus need to provide a data layout override callback that fixes the
data layout, at minimum by setting natural alignment for i8.

Any further adjustments to the module (e.g. adding padding bytes if necessary)
need to be done after module import. In the case of DXIL, this should not be
necessary, because i8 usage in DXIL is very limited and its alignment actually
does not matter, see
https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/DXIL.rst#primitive-types

Diff Detail

Event Timeline

jsilvanus created this revision.Jan 20 2023, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 6:56 AM
jsilvanus requested review of this revision.Jan 20 2023, 6:56 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 20 2023, 6:56 AM
nikic accepted this revision.Jan 20 2023, 7:50 AM
nikic added a subscriber: nikic.

LGTM

llvm/lib/IR/DataLayout.cpp
409

I'd suggest adding an AlignType == INTEGER_ALIGN && here. Probably doesn't matter in practice, but that limits it more explicitly to i8.

This revision is now accepted and ready to land.Jan 20 2023, 7:50 AM

Restrict alignment check to integer types as suggested by review.

Thanks for the review, I've updated the patch accordingly.

llvm/lib/IR/DataLayout.cpp
409

Thanks for the suggestion, I added the restriction.
I've seen some recent discussions about fp8 types, so it may not be that irrelevant after all.

This revision was landed with ongoing or failed builds.Jan 23 2023, 12:36 AM
This revision was automatically updated to reflect the committed changes.