[PATCH] D135488: [codegen] Add StackFrameLayoutAnalysisPass

Thu Jan 12 18:28:22 PST 2023

paulkirth added a comment.

In D135488#4049075 <https://reviews.llvm.org/D135488#4049075>, @thegameg wrote:

> I would rather have a more generic mechanism for remarks or diagnostics in general. Even if it uses `isFunctionInPrintList`, I'd rather have a real flag that doesn't require `-mllvm`.

Agreed. I'm going to opt into the `isFunctionInPrintList` for now. If you feel strongly, I can remove it, but it seems like a useful compromise.

In D135488#4049050 <https://reviews.llvm.org/D135488#4049050>, @nickdesaulniers wrote:

> Does name mangling complicate that? Perhaps a C++ user would give an unmangled name, but MF would be looking at mangled names?

It will absolutely expect the mangled name.  We may want to look at the other filtering facilities we have available too. Like we have those sanitizer files, and some other matching that can use regex. XRay had some logic for that stuff too, in addition to the things already in sanitizer common,  IIRC.

================
Comment at: clang/test/Frontend/stack-layout-remark.c:8
+// RUN: mkdir -p %t
+// RUN: %clang_cc1 %s -emit-codegen-only -triple x86_64-unknown-linux-gnu -target-cpu corei7 -Rpass-analysis=stack-frame-layout -o /dev/null  -O0  2>&1 | FileCheck %s --check-prefix=O0-NODEBUG
+// RUN: %clang_cc1 %s -emit-codegen-only -triple x86_64-unknown-linux-gnu -target-cpu corei7 -Rpass-analysis=stack-frame-layout -o /dev/null  -O0  -debug-info-kind=constructor  -dwarf-version=5 -debugger-tuning=gdb 2>&1 | FileCheck %s --check-prefix=O0-DEBUG
----------------
nickdesaulniers wrote:
> paulkirth wrote:
> > paulkirth wrote:
> > > nickdesaulniers wrote:
> > > > nickdesaulniers wrote:
> > > > > Please update:
> > > > > 1. the patch description/commit message
> > > > > 2. clang/docs/ReleaseNotes.rst
> > > > > 
> > > > >  to mention this new flag. I kind of wish that `-Wstack-frame-larger-than=` alluded to this somehow.
> > > > Perhaps in the documentation for `-Wframe-larger-than=`? i.e. adding a `code Documentation = [{}]` block to `BackendFrameLargerThan` record in clang/include/clang/Basic/DiagnosticGroups.td or something.
> > > Will do on the ReleaseNotes, but I'm a bit unsure what you want in the summary. It mentions the motivation and describe what this pass is for/does using remarks. Is there something else it should say?
> > > Perhaps in the documentation for `-Wframe-larger-than=`? i.e. adding a `code Documentation = [{}]` block to `BackendFrameLargerThan` record in clang/include/clang/Basic/DiagnosticGroups.td or something.
> > 
> > Hmm, I'll take a look there, but I'm not 100% sure I follow what you mean. 
> > 
> > are you after somethng like this when frame-larger-than diagnostics happen:
> > ```
> > you can debug this by adding -Rpass-analysis=stack-frame-layout -mllvm -pass-remarks-filter=<functionname>
> > ```
> > or something like that?
> > Hmm, I'll take a look there, but I'm not 100% sure I follow what you mean.
> 
> > are you after somethng like this when frame-larger-than diagnostics happen:
> 
> > you can debug this by adding -Rpass-analysis=stack-frame-layout -mllvm -pass-remarks-filter=<functionname>
> or something like that?
> 
> 
> Basically, my concern is "how will other developers not cc'ed on this phab review ever find this nifty new flag?" If -Wframe-larger-than= doesn't print info about it, then we should at least have it in our docs.
> 
> The last sentence you suggested is exactly what I had in mind.
> The last sentence you suggested is exactly what I had in mind.

Maybe we should follow this patch up w/ a change to the frame larger than diagnostic?

================
Comment at: llvm/lib/CodeGen/StackFrameLayoutAnalysisPass.cpp:220-222
+    for (MachineFunction::VariableDbgInfo &DI : MF.getVariableDbgInfo()) {
+      SlotDebugMap[DI.Slot].insert(DI.Var);
+    }
----------------
nickdesaulniers wrote:
> remove braces
> https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements
BTW, didn't clang format used to fix these? I definitely remember it doing that. I don't have any special config I use, so it's just picking up project defaults. AFAIK it's still an option, but doesn't seem to be set for the LLVM config anymore.  Did we change the behavior here for a reason?

I get caught by these a lot when a loop gets simplified.

================
Comment at: llvm/test/CodeGen/AArch64/arm64-opt-remarks-lazy-bfi.ll:43-46
+; HOTNESS:      Executing Pass 'Stack Frame Layout Analysis'
+; HOTNESS-NEXT: Freeing Pass 'Machine Optimization Remark Emitter'
+; HOTNESS-NEXT: Freeing Pass 'Lazy Machine Block Frequency Analysis'
+; HOTNESS-NEXT: Freeing Pass 'Stack Frame Layout Analysis'
----------------
nickdesaulniers wrote:
> paulkirth wrote:
> > nickdesaulniers wrote:
> > > what's going on in this test? Looks like the pass is being run twice or something?
> > not sure I follow. The block here is executing the pass then freeing the pass. I tried to follow the pattern used around this, but we could change it to
> > 
> > ```
> > HOTNESS:      Executing Pass 'Stack Frame Layout Analysis'
> > HOTNESS:      Freeing Pass 'Stack Frame Layout Analysis'
> > ```
> > and skip the rest
> Oops, I missed the first instance is `Executing` then the second is `Freeing`. NVM!
It's 100% non-obvious. It took me a bit to figure out that was how these worked when I added these.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135488