[PATCH] D135488: [codegen] Add a remarks based Stack Layout Analysis pass

Francis Visoiu Mistrih via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Jan 3 18:53:51 PST 2023


thegameg added a comment.

In D135488#4024713 <https://reviews.llvm.org/D135488#4024713>, @paulkirth wrote:

> @arsenm @thegameg @nickdesaulniers @dblaikie @phosek Can we reach a consensus here on how to output this kind of information? I feel like I've been told to move towards remarks as the output method, but that the current diagnostic that tries to lay out the stack visually isn't a good fit since remarks are also serialized ... I'm not all that convinced that providing output other than a visual layout for this information is all that useful in this particular case, but I don't have an issue with supporting it either.  I think this is especially true, since memory layouts are tricky to reason about.
>
> For that reason, I'm pretty sure we want to actually //show// the user the layout directly in the diagnostic. My concern is that if we change the output to better fit within the remarks infrastructure, we lose an effective way to show users what's happening. If we take away the visual representation, then we'll end up needing to run a separate tool and post-process the serialized output to have a user make any real sense of how things are layed out. That seems like a pretty bad user experience, so I'd much rather find a way to have the compiler emit this information directly.
>
> Does anyone have thoughts here on how to move forward?

I think remarks are the right way to go with this. They provide a pretty flexible way to emit both strings (for formatting and visual representations) and machine-readable data through `ore::NV` entries. We just need to find a consensus on how it looks like in both the command-line and the serialized mode.



================
Comment at: llvm/lib/CodeGen/StackFrameLayoutAnalysisPass.cpp:178
+
+    emitHeaderRemark(MF);
+
----------------
paulkirth wrote:
> thegameg wrote:
> > From what I can see, you've focused on the `-Rpass` output using diagnostics and tried to emit a pretty-printed version for that on the command line.
> > 
> > We use remarks through their serialized version as well, through `-fsave-optimization-record` which will emit a YAML file that can be used in scripts and other post-processing tools.
> > 
> > I think this should be something in between where it looks user-friendly on the command-line but also easy to post-process.
> > 
> > One way would be to do something similar to [[ https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Utils/MemoryOpRemark.cpp | the memory op remarks ]], which are used here: [[ https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/Util/trivial-auto-var-init-call.ll | llvm/test/Transforms/Util/trivial-auto-var-init-call.ll ]].
> > 
> > I could see something where you emit a remark for each slot (+ location), with `ore::NV` used for each piece of information that is useful, something like:
> > 
> > ```
> > ORE << MachineOptimizationRemarkAnalysis(...) << "Stack slot: offset: " << ore::NV("Offset", D.offset)
> >                                                                                     << "type: " << ore::NV("Type", type)
> > [...]
> > ```
> > 
> > and could generate something like:
> > 
> > ```
> >  --- !Analysis
> >  Pass:            stack-frame-layout
> >  Name:            StackSlot
> >  Function:        stackSizeWarning
> >  Args:
> >    - String:          'Stack slot: offset: '
> >    - Offset:         '[SP-8]'
> >    - String:          ', type: '
> >    - Type:            'spill'
> >    - String:          ', align: '
> >    - Align:           '16'
> >    - String:          ', size: '
> >    - Align:           '8'
> >  ...
> > ```
> > 
> > which would look like this on the command line:
> > 
> > ```
> > remark: Stack slot: offset: [SP-8], type: spill, align: 16, size 8
> > ```
> > 
> Thanks for the suggestion. While I understand the desire to make the output more machine readable, I don't think this is a good place to do so. Layouts are hard to reason about and there's actually a fairly decent way we can display this to users and convey exactly where things are. The entire point of this patch was to give a somewhat visual representation to how the stack is layed out, and help debug stack layout issues. It's one of the reasons I didn't originally do this with remarks, but there's been a fair amount of discussion to this point already w/in this patch. 
> 
> If this isn't a good fit for remarks with the current format, then I'm kind of stuck on how to satisfy the various requirements on how to output and display this kind of information...
> 
> 
> 
I don't see a huge difference between:

```
remark: Offset            Align     Size
remark: [SP-8]      Spill 16        8
remark: [SP-16]     Spill 8         8
remark: [SP-24]     Spill 16        8
```

and

```
remark: Stack slot: offset: [SP-8], type: spill, align: 16, size 8
remark: Stack slot: offset: [SP-16], type: spill, align: 8, size 8
remark: Stack slot: offset: [SP-24], type: spill, align: 16, size 8
```

If you think this is what makes it really useful, we also support multi-line remarks (see [[ https://github.com/llvm/llvm-project/blob/main/llvm/lib/Transforms/Scalar/LowerMatrixIntrinsics.cpp | LowerMatrixIntrinsics.cpp ]], and you can still provide precise `ore::NV`-like entries. In that case you should probably emit one big `MachineOptimizationRemarkAnalysis` with `[SP-8]`, `spill`, `16`, and `8` as `ore::NV` entries, and the rest as a strings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D135488



More information about the cfe-commits mailing list