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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
