Skip to content

[DataLayout] Use member initialization (NFC) #103712

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 14, 2024
Merged

[DataLayout] Use member initialization (NFC) #103712

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
384ff1a
[DataLayout] Use member initialization (NFC)
s-barannikov Aug 13, 2024
c5c7fde
Make default specs local to cpp file
s-barannikov Aug 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 19 additions & 24 deletions llvm/include/llvm/IR/DataLayout.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,6 @@ enum AlignTypeEnum {
/// Layout alignment element.
///
/// Stores the alignment data associated with a given type bit width.
///
/// \note The unusual order of elements in the structure attempts to reduce
/// padding and make the structure slightly more cache friendly.
struct LayoutAlignElem {
uint32_t TypeBitWidth;
Align ABIAlign;
Expand All @@ -82,14 +79,11 @@ struct LayoutAlignElem {
/// Layout pointer alignment element.
///
/// Stores the alignment data associated with a given pointer and address space.
///
/// \note The unusual order of elements in the structure attempts to reduce
/// padding and make the structure slightly more cache friendly.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment goes back to the times when the fields were bitfields. It is no longer relevant.

struct PointerAlignElem {
uint32_t AddressSpace;
uint32_t TypeBitWidth;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea of this order change is to match how it's written in the string representation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

Align ABIAlign;
Align PrefAlign;
uint32_t TypeBitWidth;
uint32_t AddressSpace;
uint32_t IndexBitWidth;

/// Initializer
Expand All @@ -115,16 +109,16 @@ class DataLayout {
MultipleOfFunctionAlign,
};
private:
/// Defaults to false.
bool BigEndian;
bool BigEndian = false;

unsigned AllocaAddrSpace;
unsigned ProgramAddrSpace;
unsigned DefaultGlobalsAddrSpace;
unsigned AllocaAddrSpace = 0;
unsigned ProgramAddrSpace = 0;
unsigned DefaultGlobalsAddrSpace = 0;

MaybeAlign StackNaturalAlign;
MaybeAlign FunctionPtrAlign;
FunctionPtrAlignType TheFunctionPtrAlignType;
FunctionPtrAlignType TheFunctionPtrAlignType =
FunctionPtrAlignType::Independent;

enum ManglingModeT {
MM_None,
Expand All @@ -136,24 +130,22 @@ class DataLayout {
MM_Mips,
MM_XCOFF
};
ManglingModeT ManglingMode;
ManglingModeT ManglingMode = MM_None;

// FIXME: `unsigned char` truncates the value parsed by `parseSpecifier`.
SmallVector<unsigned char, 8> LegalIntWidths;

/// Primitive type alignment data. This is sorted by type and bit
/// width during construction.
using AlignmentsTy = SmallVector<LayoutAlignElem, 4>;
AlignmentsTy IntAlignments;
AlignmentsTy FloatAlignments;
AlignmentsTy VectorAlignments;
// Primitive type specifications. Sorted and uniqued by type bit width.
SmallVector<LayoutAlignElem, 6> IntAlignments;
SmallVector<LayoutAlignElem, 4> FloatAlignments;
SmallVector<LayoutAlignElem, 10> VectorAlignments;

// Pointer type specifications. Sorted and uniqued by address space number.
SmallVector<PointerAlignElem, 8> Pointers;

/// The string representation used to create this DataLayout
std::string StringRepresentation;

using PointersTy = SmallVector<PointerAlignElem, 8>;
PointersTy Pointers;

const PointerAlignElem &getPointerAlignElem(uint32_t AddressSpace) const;

// Struct type ABI and preferred alignments. The default spec is "a:8:64".
Expand Down Expand Up @@ -189,6 +181,9 @@ class DataLayout {
Error parseSpecifier(StringRef Desc);

public:
/// Constructs a DataLayout with default values.
DataLayout();

/// Constructs a DataLayout from a specification string.
/// WARNING: Aborts execution if the string is malformed. Use parse() instead.
explicit DataLayout(StringRef LayoutString);
Expand Down
59 changes: 30 additions & 29 deletions llvm/lib/IR/DataLayout.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,37 +198,38 @@ const char *DataLayout::getManglingComponent(const Triple &T) {
return "-m:e";
}

static const std::pair<AlignTypeEnum, LayoutAlignElem> DefaultAlignments[] = {
{INTEGER_ALIGN, {1, Align(1), Align(1)}}, // i1
{INTEGER_ALIGN, {8, Align(1), Align(1)}}, // i8
{INTEGER_ALIGN, {16, Align(2), Align(2)}}, // i16
{INTEGER_ALIGN, {32, Align(4), Align(4)}}, // i32
{INTEGER_ALIGN, {64, Align(4), Align(8)}}, // i64
{FLOAT_ALIGN, {16, Align(2), Align(2)}}, // half, bfloat
{FLOAT_ALIGN, {32, Align(4), Align(4)}}, // float
{FLOAT_ALIGN, {64, Align(8), Align(8)}}, // double
{FLOAT_ALIGN, {128, Align(16), Align(16)}}, // ppcf128, quad, ...
{VECTOR_ALIGN, {64, Align(8), Align(8)}}, // v2i32, v1i64, ...
{VECTOR_ALIGN, {128, Align(16), Align(16)}}, // v16i8, v8i16, v4i32, ...
// Default primitive type specifications.
// NOTE: These arrays must be sorted by type bit width.
constexpr LayoutAlignElem DefaultIntSpecs[] = {
{1, Align::Constant<1>(), Align::Constant<1>()}, // i1:8:8
{8, Align::Constant<1>(), Align::Constant<1>()}, // i8:8:8
{16, Align::Constant<2>(), Align::Constant<2>()}, // i16:16:16
{32, Align::Constant<4>(), Align::Constant<4>()}, // i32:32:32
{64, Align::Constant<4>(), Align::Constant<8>()}, // i64:32:64
};
constexpr LayoutAlignElem DefaultFloatSpecs[] = {
{16, Align::Constant<2>(), Align::Constant<2>()}, // f16:16:16
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes actually hard codes how many bytes that correspond to 16 bits. If the goal with this patch was to simplify life for non-8-bit-byte targets (as you wrote that you planned to do in https://discourse.llvm.org/t/rfc-on-non-8-bit-bytes-and-the-target-for-it/53455/46) then I'm confused. This currently causes some head-ache for use downstream as our old solution for converting the bit-sized alignments into bytes no longer works.

Should there perhaps be a comment somewhere saying that the alignments are specified in "number of octests"? And why is that better than specifying them in bits?

(Btw, as I mentioned in the discourse thread I wouldn't mind being added as reviewer to the patches related to non-8-bit-byte-support-improvements. This to avoid surprises like this when merging the patches that already has landed on main.)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I see now that the alignments weren't specified in bits before this patch either. That was just in our downstream fork.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The end goal is indeed to simplify life for non-8-bit targets.
I'd like to do some NFC refactoring and improve test coverage before posting an RFC patch that adds the support.
I'm very sorry for the conflicts.

the alignments weren't specified in bits before this patch either.

Yep, this patch didn't change that.

Should there perhaps be a comment somewhere saying that the alignments are specified in "number of octests"? And why is that better than specifying them in bits?

Storing alignments in bits in one possible solution (and in fact I'm doing this downstream, too). It might look attractive, but it has some downsides. Let's discuss this in the discourse thread.

{32, Align::Constant<4>(), Align::Constant<4>()}, // f32:32:32
{64, Align::Constant<8>(), Align::Constant<8>()}, // f64:64:64
{128, Align::Constant<16>(), Align::Constant<16>()}, // f128:128:128
};
constexpr LayoutAlignElem DefaultVectorSpecs[] = {
{64, Align::Constant<8>(), Align::Constant<8>()}, // v64:64:64
{128, Align::Constant<16>(), Align::Constant<16>()}, // v128:128:128
};

DataLayout::DataLayout(StringRef LayoutString) {
BigEndian = false;
AllocaAddrSpace = 0;
ProgramAddrSpace = 0;
DefaultGlobalsAddrSpace = 0;
TheFunctionPtrAlignType = FunctionPtrAlignType::Independent;
ManglingMode = MM_None;

// Default alignments
for (const auto &[Kind, Layout] : DefaultAlignments) {
if (Error Err = setAlignment(Kind, Layout.ABIAlign, Layout.PrefAlign,
Layout.TypeBitWidth))
report_fatal_error(std::move(Err));
}
if (Error Err = setPointerAlignmentInBits(0, Align(8), Align(8), 64, 64))
report_fatal_error(std::move(Err));
// Default pointer type specifications.
constexpr PointerAlignElem DefaultPointerSpecs[] = {
{0, 64, Align::Constant<8>(), Align::Constant<8>(), 64} // p0:64:64:64:64
};

DataLayout::DataLayout()
: IntAlignments(ArrayRef(DefaultIntSpecs)),
FloatAlignments(ArrayRef(DefaultFloatSpecs)),
VectorAlignments(ArrayRef(DefaultVectorSpecs)),
Pointers(ArrayRef(DefaultPointerSpecs)) {}

DataLayout::DataLayout(StringRef LayoutString) : DataLayout() {
if (Error Err = parseSpecifier(LayoutString))
report_fatal_error(std::move(Err));
}
Expand Down Expand Up @@ -277,7 +278,7 @@ bool DataLayout::operator==(const DataLayout &Other) const {
}

Expected<DataLayout> DataLayout::parse(StringRef LayoutDescription) {
DataLayout Layout("");
DataLayout Layout;
if (Error Err = Layout.parseSpecifier(LayoutDescription))
return std::move(Err);
return Layout;
Expand Down
2 changes: 1 addition & 1 deletion llvm/lib/IR/Module.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ template class llvm::SymbolTableListTraits<GlobalIFunc>;

Module::Module(StringRef MID, LLVMContext &C)
: Context(C), ValSymTab(std::make_unique<ValueSymbolTable>(-1)),
ModuleID(std::string(MID)), SourceFileName(std::string(MID)), DL(""),
ModuleID(std::string(MID)), SourceFileName(std::string(MID)),
IsNewDbgInfoFormat(UseNewDbgInfoFormat) {
Context.addModule(this);
}
Expand Down
2 changes: 1 addition & 1 deletion llvm/unittests/Analysis/ValueLatticeTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ namespace {
class ValueLatticeTest : public testing::Test {
protected:
LLVMContext Context;
DataLayout DL = DataLayout("");
DataLayout DL;
};

TEST_F(ValueLatticeTest, ValueLatticeGetters) {
Expand Down
5 changes: 2 additions & 3 deletions llvm/unittests/CodeGen/LowLevelTypeTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ namespace {

TEST(LowLevelTypeTest, Token) {
LLVMContext C;
DataLayout DL("");

const LLT TTy = LLT::token();

Expand All @@ -38,7 +37,7 @@ TEST(LowLevelTypeTest, Token) {

TEST(LowLevelTypeTest, Scalar) {
LLVMContext C;
DataLayout DL("");
DataLayout DL;

for (unsigned S : {0U, 1U, 17U, 32U, 64U, 0xfffffU}) {
const LLT Ty = LLT::scalar(S);
Expand Down Expand Up @@ -70,7 +69,7 @@ TEST(LowLevelTypeTest, Scalar) {

TEST(LowLevelTypeTest, Vector) {
LLVMContext C;
DataLayout DL("");
DataLayout DL;

for (unsigned S : {0U, 1U, 17U, 32U, 64U, 0xfffU}) {
for (auto EC :
Expand Down
4 changes: 2 additions & 2 deletions llvm/unittests/IR/VectorTypesTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ TEST(VectorTypesTest, BaseVectorType) {

TEST(VectorTypesTest, FixedLenComparisons) {
LLVMContext Ctx;
DataLayout DL("");
DataLayout DL;

Type *Int32Ty = Type::getInt32Ty(Ctx);
Type *Int64Ty = Type::getInt64Ty(Ctx);
Expand Down Expand Up @@ -322,7 +322,7 @@ TEST(VectorTypesTest, FixedLenComparisons) {

TEST(VectorTypesTest, ScalableComparisons) {
LLVMContext Ctx;
DataLayout DL("");
DataLayout DL;

Type *Int32Ty = Type::getInt32Ty(Ctx);
Type *Int64Ty = Type::getInt64Ty(Ctx);
Expand Down
Loading