[PATCH] D78672: [Debuginfo] Remove redundand variable from getAttributeValue()

[Greg Clayton via Phabricator via llvm-commits]

clayborg added a comment.

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.



================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:153
     const DWARFUnit &U) const {
   Optional<uint32_t> MatchAttrIndex = findAttributeIndex(Attr);
   if (!MatchAttrIndex)
----------------
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:156
     return None;
 
   auto DebugInfoData = U.getDebugInfoExtractor();
----------------
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.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFAbbreviationDeclaration.cpp:162-180
   for (const auto &Spec : AttributeSpecs) {
-    if (*MatchAttrIndex == AttrIndex) {
+    if (Spec.Attr == Attr) {
       // We have arrived at the attribute to extract, extract if from Offset.
       if (Spec.isImplicitConst())
         return DWARFFormValue::createFromSValue(Spec.Form,
                                                 Spec.getImplicitConstValue());
 
----------------
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;
```


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78672/new/

https://reviews.llvm.org/D78672