[PATCH] D130875: [Metadata] Introduce MD_pcsections

[Marco Elver via Phabricator via llvm-commits]

melver marked 3 inline comments as done.
melver added inline comments.


================
Comment at: llvm/lib/IR/MDBuilder.cpp:166
+
+  for (size_t i = 0; i < Sections.size(); i++) {
+    const StringRef &S = Sections[i];
----------------
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.


================
Comment at: llvm/lib/IR/MDBuilder.cpp:174
+      for (Constant *C : AuxConsts) {
+        if (C)
+          Ops.push_back(createConstant(C));
----------------
vitalybuka wrote:
> why do you need nullptr in that array?
I don't think we do.


================
Comment at: llvm/test/CodeGen/AArch64/pcsections.ll:1
+; BROKEN: llc -O0 < %s | FileCheck %s --check-prefixes=CHECK,CHECK-UNOPT,DEFCM
+; BROKEN: llc -O1 < %s | FileCheck %s --check-prefixes=CHECK,CHECK-OPT,DEFCM
----------------
MaskRay wrote:
> melver wrote:
> > melver wrote:
> > > MaskRay wrote:
> > > > One RUN line for the default code model suffices. There is no codegen optimization difference.
> > > > 
> > > > A patch should not introduce disabled tests this way. If a dependent patch introduces the functionality, the test should be added to that patch.
> > > > 
> > > I don't understand. Removing any one line here will reduce coverage.
> > > 
> > > We have RUNs for each -O0 to -O3 with the default code model, and then only O1 with a large code model.
> > Note that e.g. on arm64 -O0 uses GlobalISel and -O1 and above use FastISel+SelectionDAG, so using different optimization levels will test different ISels. Different optimization levels have helped me catch bugs in earlier implementations.
> To exercise all of GlobalISel FastISel SelectionDAG, the best way is to use explicit options like `-fast-isel=`, `-global-isel=`, instead of relying on -O0/-O1/-O2/-O3. -O* runs a large set of codegen pipeline which is pretty irrelevant to this patch.
> 
> For a patch series, it's not suitable to add `BROKEN:` in patch a and change them to `RUN:` in patch b. This should be refactored to just add `RUN:` in patch b. If you feel that showding the diff is important, change a to add RUN: about the current state and change b to show the difference.
In the latest diff of this patch the tests are gone and I moved them to the AsmPrinter patch (now later in the series).

I will additionally add tests to explicitly exercise each ISel there, but want to keep -O* for the additional coverage (whether or not there are irrelevant optimizations being run today, in future this may change).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130875