[Debuginfo] Remove redundand variable from getAttributeValue()

[avl]

Summary

AttrIndex could be removed from DWARFAbbreviationDeclaration::getAttributeValue.

[clayborg]

So my only issue is adding a comment so people don't remove the code that quickly checks if this abbreviation has the attribute. This is a quick check that doesn't require manually skipping each form size only to discover that we don't have the form.

All other comments are suggestions for making attribute value accesses more performant.

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

155

Might be worth adding a comment here since MatchAttrIndex is no longer used later on if the function. Comment like:

// Check if this abbreviation has this attribute without needing to skip any data so we can return quickly if it doesn't.

Just want to make sure people don't later take this line and the following two lines out.

158

If we tracked the fixed offset from the start of the DIE inside each AttributeSpec with a new ivar: "Optional<uint32_t> FixedOffset;", this code could be made more efficient by directly accessing the attribute value's offset and extracting the value:

const auto &Spec = AttributeSpec[*MatchAttrIndex];
if (Spec.FixedOffset) {
  uint64_t Offset = *Spec.FixedOffset;
  // We have arrived at the attribute to extract, extract if from Offset.
  if (Spec.isImplicitConst())
    return DWARFFormValue::createFromSValue(Spec.Form,
                                                Spec.getImplicitConstValue());

    DWARFFormValue FormValue(Spec.Form);
    if (FormValue.extractValue(DebugInfoData, &Offset, U.getFormParams(), &U))
      return FormValue;
}

If the AttributeSpec::FixedOffset is valid, then we can use it, else we would need to iterate manually like below. The new FixedOffset would include the CodeByteSize for each attribute. As soon as we run into an attribute that does not have a fixed size, then the subsequent and all following attributes would have invalid FixedOffset ivars. This would allow direct access to attribute values in a lot of cases with a little more work when making AttributeSpec array in the extract routine. We are already tracking fixed sizes of form values, so it wouldn't be hard to do.

163–182

If we did end up tracking FixedOffset values in each AttributeSpec, we could track index of the last AttributeSpec that has a fixed form size and keep it as an ivar to this class. This loop could become more efficient by skipping all of the fixed form sizes up front and then only iterating over the remaining ones:

uint64_t Offset = *AttributeSpecs[LastFixedAttrIdx].FixedOffset;
for (i=LastFixedAttrIdx; i<* MatchAttrIndex; ++i) {
  if (auto FixedSize = AttributeSpecs[i].getByteSize(U))
    Offset += *FixedSize;
  else
    DWARFFormValue::skipValue(AttributeSpecs[i].Form, DebugInfoData, &Offset, U.getFormParams());
}
const auto &Spec = AttributeSpecs[i];
if (Spec.isImplicitConst())
  return DWARFFormValue::createFromSValue(Spec.Form, Spec.getImplicitConstValue());

DWARFFormValue FormValue(Spec.Form);
if (FormValue.extractValue(DebugInfoData, &Offset, U.getFormParams(), &U))
  return FormValue;
return None;

[dblaikie]

I'd be inclined to rephrase this a bit & actually use MatchAttrIndex more rather than less - to make the fast path clearer/more integrated, rather than less (given @clayborg's concerns).

For instance, that "return None" at the end of the function should be unreachable, right? We should never make it through the for loop without at some point satisfying the condition... oh, but we would if we failed to extract the value - but if we did, should we keep going looking for other instances of the same attribute? (the old code kept looping, but would never find another, the new code keeps looping and might find another instance of the attribute - the DWARF spec says that's invalid, but I'm not sure any of our parsers enforce that? Maybe LLDB's does?)

I'm imagining something more like:

Optional<uint32_t> MatchAttrIndex = findAttributeIndex(Attr);
if (!MatchAttrIndex)
  return None;

for (int i = 0; i != *MatchAttrIndex; ++i) {
  if (auto FixedSize = Spec.getByteSize(U))
    Offset += *FixedSize;
  else
    DWARFFormValue::skipValue(...);
}

// We have arrived at the attribute to extract, extract if from Offset.
if (Spec.isImplicitConst())
  return DWARFFormValue::createFromSValue(...);

DWARFFormValue FormValue(Spec.Form);
if (FormValue.extractValue(DebugInfoData, &Offset, U.getFormParams(), &U))
  return FormValue;

return None;

[avl]

addressed comments(added comment, refactored navigating loop).

[avl]

@clayborg Thank you for the ideas on performance optimization. I would evaluate them and create additional review if there would be good results.

[dblaikie]

Looks good to me - welcome to wait for @clayborg to sign off too if you like.

[clayborg]

Yes, much cleaner! I look forward to seeing any perf optimizations in the future if they prove to be faster. It really depends on how many workflows directly access attributes. It is probably quite common for symbolication where it will look for low and high pc and name.

[clayborg]

Thinking from the compiler side: it would be really cool if the compiler could emit all variable sized DWARF attributes at the end of each DIE to maximize how many constant attribute values accesses we could do (if we implement the O(1) access to the value perf optimization in the debug info).

[probinson]

In D78672#2000012, @clayborg wrote:

Thinking from the compiler side: it would be really cool if the compiler could emit all variable sized DWARF attributes at the end of each DIE to maximize how many constant attribute values accesses we could do (if we implement the O(1) access to the value perf optimization in the debug info).

Would need to sort the attributes after we were done building the DIEs; it isn't really feasible to do this while we're figuring out what info belongs in the DIE. Identifying which forms are fixed-size clearly wouldn't be hard, just slightly tedious.

[avl]

Yes, much cleaner! I look forward to seeing any perf optimizations in the future if they prove to be faster. It really depends on how many workflows directly access attributes. It is probably quite common for symbolication where it will look for low and high pc and name.

I also have in mind some optimization which would limit number of passes through attributes values. It is often necessary to get not only single attribute, but pair or triple. Currently it leads to several calls to find() and then several passes through attributes values. It is generally possible to get all required attributes through the one pass.

[avl]

@clayborg

If we did end up tracking FixedOffset values in each AttributeSpec, we could track index of the last AttributeSpec that has a fixed form size and keep it as an ivar to this class. This loop could become more efficient by skipping all of the fixed form sizes up front and then only iterating over the remaining ones:

I gave it a try. But it did not show any performance improvement. It even created a small(0.5%) performance degradation - https://reviews.llvm.org/differential/diff/261331/

[clayborg]

In D78672#2013690, @avl wrote:

@clayborg

If we did end up tracking FixedOffset values in each AttributeSpec, we could track index of the last AttributeSpec that has a fixed form size and keep it as an ivar to this class. This loop could become more efficient by skipping all of the fixed form sizes up front and then only iterating over the remaining ones:

I gave it a try. But it did not show any performance improvement. It even created a small(0.5%) performance degradation - https://reviews.llvm.org/differential/diff/261331/

Thanks for trying it! Always good to verify numbers are better when trying to improve things.

llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp

if (AttributeSpecs[i].Attr == Attr)
       return i;
   }
   return None;
 }
 
 Optional<DWARFFormValue> DWARFAbbreviationDeclaration::getAttributeValue(
     const uint64_t DIEOffset, const dwarf::Attribute Attr,
     const DWARFUnit &U) const {
+  // Check if this abbreviation has this attribute without needing to skip
+  // any data so we can return quickly if it doesn't.
   Optional<uint32_t> MatchAttrIndex = findAttributeIndex(Attr);
   if (!MatchAttrIndex)
     return None;
 
   auto DebugInfoData = U.getDebugInfoExtractor();
 
   // Add the byte size of ULEB that for the abbrev Code so we can start
   // skipping the attribute data.
   uint64_t Offset = DIEOffset + CodeByteSize;
-  uint32_t AttrIndex = 0;
-  for (const auto &Spec : AttributeSpecs) {
-    if (*MatchAttrIndex == AttrIndex) {
-      // We have arrived at the attribute to extract, extract if from Offset.
-      if (Spec.isImplicitConst())
-        return DWARFFormValue::createFromSValue(Spec.Form,
-                                                Spec.getImplicitConstValue());
+  for (uint32_t CurAttrIdx = 0; CurAttrIdx != *MatchAttrIndex; ++CurAttrIdx)
+    // Match Offset along until we get to the attribute we want.
+    if (auto FixedSize = AttributeSpecs[CurAttrIdx].getByteSize(U))
+      Offset += *FixedSize;
+    else
+      DWARFFormValue::skipValue(AttributeSpecs[CurAttrIdx].Form, DebugInfoData,
+                                &Offset, U.getFormParams());
+
+  const AttributeSpec &Spec = AttributeSpecs[*MatchAttrIndex];
 
-      DWARFFormValue FormValue(Spec.Form);
-      if (FormValue.extractValue(DebugInfoData, &Offset, U.getFormParams(), &U))
-        return FormValue;
-    }
-    // March Offset along until we get to the attribute we want.
-    if (auto FixedSize = Spec.getByteSize(U))
-      Offset += *FixedSize;
-    else
-      DWARFFormValue::skipValue(Spec.Form, DebugInfoData, &Offset,
-                                U.getFormParams());
-    ++AttrIndex;
-  }
+
   return None;
 }
 
 size_t DWARFAbbreviationDeclaration::FixedSizeInfo::getByteSize(
     const DWARFUnit &U) const {
   size_t ByteSize = NumBytes;
   if (NumAddrs)
     ByteSize += NumAddrs * U.getAddressByteSize();
   if (NumRefAddrs)