[PATCH] D112282: [llvm-profgen] Switch to DWARF-based symbol and ranges

[Lei Wang via Phabricator via llvm-commits]

wlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:499
     uint64_t EndOffset = I.second;
     Binary->computeInlinedContextSizeForRange(StartOffset, EndOffset);
   }
----------------
hoy wrote:
> From the way ranges of a dwarf symbol are populated, `EndOffset` should not be a part of a range (exclusive). But `computeInlinedContextSizeForRange` expects the opposite.
Good point, I just changed EndOffset inclusive range.
```
uint64_t EndOffset = Range.HighPC - getPreferredBaseAddress() - 1;
```


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:336
                     : SectionOffset + SectSize;
-  if (StartOffset >= NextStartOffset)
+  if (StartOffset > NextStartOffset)
     return true;
----------------
hoy wrote:
> why remove the equal compare? A label at the end of the text section has StartOffset == NextStartOffset.
this is because I saw some symbols has the same StartOffset but different name and mismatch its dwarf info. for example:
```
[0x01] main
[0x01] foo
[0x02] move ...       main:1 
```

both main and foo's startoffset is [0x01], previously we will skip main, but in a rare case, the body of the function is actually from main not foo( main's the dwarf range is not zero), later we need to set main as the entry of the function. So here I tried to cover this case,the only cost is we process is the zero-size symbol for setting the entry of function.



================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.cpp:570
+        uint64_t FunctionStart = Range.LowPC;
+        uint64_t FunctionSize = Range.HighPC - FunctionStart;
+
----------------
hoy wrote:
> wlei wrote:
> > hoy wrote:
> > > Is `HighPC` inclusive or not?
> > it's inclusive, it's the same as the previous EndOffset in symbol table.
> I see. It actually means exclusive which means HighPC does not belong to the current function :). Otherwise `FunctionSize` should be `Range.HighPC - FunctionStart + 1`.
I see, you're right. it's also not the next start offset. I saw in spec2017, the `HighPC` of main is 2058d7

```
<main>:
  ...
  2058cf: leal  (%rdx,%rdx), %eax                 cost_compare:11
  2058d2: addl  $-1, %eax                         cost_compare:11
  2058d5: popq  %rbp
  2058d6: retq
  2058d7: int3.    # HighPC
  2058d8: int3
  2058d9: int3
  2058da: int3
  2058db: int3
  2058dc: int3
  2058dd: int3
  2058de: int3
  2058df: int3

<spec_qsort>:
  2058e0: pushq %rbp                             spec_qsort:1
  2058e1: movq  %rsp, %rbp                        spec_qsort:1
  2058e4: pushq %r15     
```


================
Comment at: llvm/tools/llvm-profgen/ProfiledBinary.h:182
+  // A vector of DWARF ranges used to quick lookup for all the ranges from
+  // different part of function.
+  std::vector<DWARFRangesVectorTy> AllDWARFRanges;
----------------
hoy wrote:
> nit: different parts of a function
fixed, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112282