[PATCH] D130875: [Metadata] Introduce MD_pcsections

[Marco Elver via Phabricator via llvm-commits]

melver added inline comments.


================
Comment at: llvm/docs/PCSectionsMetadata.rst:29
+    !"<section#1>"
+    [ , iXX <aux-consts#1> ... ]
+    [ !"<section#2">
----------------
vitalybuka wrote:
> can aux-consts be just an aggregate?
> 
> so you have more structured format: section, array, section, array, section, array,....
Yes it can - it's a trade-off. Making it another MDTuple makes interpreting and constructing the MDNode more complex. But it might save space if the same auxiliary data is reused for different sections.

On a whole, placing the auxiliary data into a separate tuple seems nicer, so I've changed the format. However, I feel there's no point in artificially restricting only 1 tuple after a section string, so things like ``!0 = !{"sec1", !1, !2, "sec2", ..}`` will be allowed (distinguishing MDString and MDNode within the outer tuple is unambiguous).


================
Comment at: llvm/lib/IR/MDBuilder.cpp:166
+
+  for (size_t i = 0; i < Sections.size(); i++) {
+    const StringRef &S = Sections[i];
----------------
vitalybuka wrote:
> vitalybuka wrote:
> > 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());
> > ```
> > Map doesn't have stable iteration order.
> Do you need a particular section order?
> 
> pair<stringref, SmallVector> is not nice, but better than non-obvious mapping.
> 
> maybe real builder?
> ```
> PCSectionsBuilder B =  MDB.getPCSectionsBuilder();
> F.setMetadata(
>    B.addSection(S, {c1})
>       .addSection(S, {}).
>       .addSection(S, {c1, c2})
>       .build());
> ```
> 
> 
We need order for deterministic compilation and tests. :-/

The main downside of doing pair<StringRef, SmallVector> is a little bit of wasted space. A builder won't help with that and for now just seems more complex. The easiest seems to be to just define MDBuilder::PCSection which users can use as entry type for any type of vector or array.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130875