[PATCH] D130875: [Metadata] Introduce MD_pcsections

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka added inline comments.


================
Comment at: llvm/docs/PCSectionsMetadata.rst:29
+    !"<section#1>"
+    [ , iXX <aux-consts#1> ... ]
+    [ !"<section#2">
----------------
can aux-consts be just an aggregate?

so you have more structured format: section, array, section, array, section, array,....


================
Comment at: llvm/lib/IR/MDBuilder.cpp:166
+
+  for (size_t i = 0; i < Sections.size(); i++) {
+    const StringRef &S = Sections[i];
----------------
melver wrote:
> vitalybuka wrote:
> > is implicit mapping from Sections[i] to AuxData[i] in interface is neccecary?
> > can you do SomeMap<StringRef, <ArrayRef<Constant *>> ?
> > 
> > That would be cleaner
> Map doesn't have stable iteration order. So I'm trying ArrayRef<std::pair<StringRef, SmallVector<Constant*>>>. The problem is that the inner array can't be an ArrayRef, otherwise users of the function have to either convert to something that can convert to ArrayRef<std::pair<StringRef, ArrayRef>> ensuring that whatever inner ref they pass lives long enough.
> 
> The reason of the earlier interface was to avoid forcing SmallVector or any particular vector onto callers by sticking to ArrayRef. But by wrapping it into a std::pair, the ArrayRef conversion can't be done implicitly anymore, so usability suffers.
> 
> This means that in SanitizerBinaryMetadadata, I'm now using a SmallVector<StringRef, SmallVector<Constant*>>> (instead of just a SmallVector<StringRef>)  where the inner SmallVector will always be 0-sized, which wastes space but didn't before. Perhaps this was premature optimization.
>> Map doesn't have stable iteration order.
Do you need a particular section order?

pair<stringref, SmallVector> is not nice, but better than obvious mapping.


maybe real builder?
```
PCSectionsBuilder B =  MDB.getPCSectionsBuilder();
F.setMetadata(
   B.addSection(S, {c1})
      .addSection(S, {}).
      .addSection(S, {c1, c2})
      .buid());
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130875