[PATCH] D130875: [Metadata] Introduce MD_pcsections

Thu Aug 4 10:42:35 PDT 2022

MaskRay added inline comments.

================
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
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130875