Page MenuHomePhabricator
Differential D36562 Diff 110641 lib/CodeGen/CGRecordLayoutBuilder.cpp

Changeset View

Standalone View

View Options

lib/CodeGen/CGRecordLayoutBuilder.cpp

Show First 20 Lines • Show All 397 Lines • ▼ Show 20 Lines for (; Field != FieldEnd; ++Field) {
} }
// Bitfields get the offset of their storage but come afterward and remain // Bitfields get the offset of their storage but come afterward and remain
// there after a stable sort. // there after a stable sort.
Members.push_back(MemberInfo(bitsToCharUnits(StartBitOffset), Members.push_back(MemberInfo(bitsToCharUnits(StartBitOffset),
MemberInfo::Field, nullptr, *Field)); MemberInfo::Field, nullptr, *Field));
} }
return; return;
} }
// Check if current Field is better to be a single field run.
// When current field has legal integer width, and its bitfield offset is
// naturally aligned, it is better to make the bitfield a separate storage
// component so as it can be accessed directly.
// Note: We don't separate bitfield in the middle of a run of fields
hfinkelUnsubmitted
Done
ReplyInline Actions

betterBeSingleFieldRun -> IsBetterAsSingleFieldRun

hfinkel: betterBeSingleFieldRun -> IsBetterAsSingleFieldRun
// because it is possible to hinder bitfields accesses combine. We only
// separate bitfield at the beginning of a run.
auto betterBeSingleFieldRun = [&](RecordDecl::field_iterator Field) {
if (!DataLayout.isLegalInteger(Field->getBitWidthValue(Context)))
return false;
// Make sure Field is natually aligned if it is treated as an IType integer.
llvm::Type *IType = getIntNType(Field->getBitWidthValue(Context));
if (getFieldBitOffset(*Field) % Context.toBits(getAlignment(IType)) != 0)
return false;
return true;
};
bool SingleFieldRun = false;
for (;;) { for (;;) {
// Check to see if we need to start a new run. // Check to see if we need to start a new run.
if (Run == FieldEnd) { if (Run == FieldEnd) {
// If we're out of fields, return. // If we're out of fields, return.
if (Field == FieldEnd) if (Field == FieldEnd)
break; break;
// Any non-zero-length bitfield can start a new run. // Any non-zero-length bitfield can start a new run.
if (Field->getBitWidthValue(Context) != 0) { if (Field->getBitWidthValue(Context) != 0) {
Run = Field; Run = Field;
StartBitOffset = getFieldBitOffset(*Field); StartBitOffset = getFieldBitOffset(*Field);
Tail = StartBitOffset + Field->getBitWidthValue(Context); Tail = StartBitOffset + Field->getBitWidthValue(Context);
SingleFieldRun = betterBeSingleFieldRun(Run);
} }
++Field; ++Field;
continue; continue;
} }
// Add bitfields to the run as long as they qualify. // Add bitfields to the run as long as they qualify.
if (Field != FieldEnd && Field->getBitWidthValue(Context) != 0 && if (!SingleFieldRun && Field != FieldEnd &&
Field->getBitWidthValue(Context) != 0 &&
Tail == getFieldBitOffset(*Field)) { Tail == getFieldBitOffset(*Field)) {
Tail += Field->getBitWidthValue(Context); Tail += Field->getBitWidthValue(Context);
++Field; ++Field;
continue; continue;
hfinkelUnsubmitted
Not Done
ReplyInline Actions

The logic here is not obvious. Can you please add a comment. SingleFieldRun here is only not equal to betterBeSingleFieldRun(Field) if we've skipped 0-length bitfields, right? Please explain what's going on and also please make sure there's a test case.

hfinkel: The logic here is not obvious. Can you please add a comment. SingleFieldRun here is only not…
wmiAuthorUnsubmitted
Not Done
ReplyInline Actions

I restructure the code a little bit and hope the logic is more clear. I already have a testcase added for it.

wmi: I restructure the code a little bit and hope the logic is more clear. I already have a testcase…
} }
// We've hit a break-point in the run and need to emit a storage field. // We've hit a break-point in the run and need to emit a storage field.
llvm::Type *Type = getIntNType(Tail - StartBitOffset); llvm::Type *Type = getIntNType(Tail - StartBitOffset);
hfinkelUnsubmitted
Done
ReplyInline Actions

Why do you have the IsBetterAsSingleFieldRun(Run) check here (where we'll evaluate it multiple times (for all of the fields in the run)). Can't you make the predicate above directly?

// Any non-zero-length bitfield can start a new run.
if (Field->getBitWidthValue(Context) != 0 &&
     !IsBetterAsSingleFieldRun(Field)) {
  Run = Field;
  StartBitOffset = getFieldBitOffset(*Field);
  ...
hfinkel: Why do you have the `IsBetterAsSingleFieldRun(Run)` check here (where we'll evaluate it…
// Add the storage member to the record and set the bitfield info for all of // Add the storage member to the record and set the bitfield info for all of
// the bitfields in the run. Bitfields get the offset of their storage but // the bitfields in the run. Bitfields get the offset of their storage but
// come afterward and remain there after a stable sort. // come afterward and remain there after a stable sort.
Members.push_back(StorageInfo(bitsToCharUnits(StartBitOffset), Type)); Members.push_back(StorageInfo(bitsToCharUnits(StartBitOffset), Type));
for (; Run != Field; ++Run) for (; Run != Field; ++Run)
Members.push_back(MemberInfo(bitsToCharUnits(StartBitOffset), Members.push_back(MemberInfo(bitsToCharUnits(StartBitOffset),
MemberInfo::Field, nullptr, *Run)); MemberInfo::Field, nullptr, *Run));
Run = FieldEnd; Run = FieldEnd;
SingleFieldRun = false;
} }
} }
void CGRecordLowering::accumulateBases() { void CGRecordLowering::accumulateBases() {
// If we've got a primary virtual base, we need to add it with the bases. // If we've got a primary virtual base, we need to add it with the bases.
if (Layout.isPrimaryBaseVirtual()) { if (Layout.isPrimaryBaseVirtual()) {
const CXXRecordDecl *BaseDecl = Layout.getPrimaryBase(); const CXXRecordDecl *BaseDecl = Layout.getPrimaryBase();
Members.push_back(MemberInfo(CharUnits::Zero(), MemberInfo::Base, Members.push_back(MemberInfo(CharUnits::Zero(), MemberInfo::Base,
▲ Show 20 Lines • Show All 415 Lines • Show Last 20 Lines