paulkirth added a comment.
@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?
================
Comment at: llvm/lib/CodeGen/StackFrameLayoutAnalysisPass.cpp:92
+ &MF.front())
+ << "FunctionName: " << ore::NV("FunctionName", MF.getName());
+ });
----------------
thegameg wrote:
> Why are we emitting the function name? In the serialized remarks
> (`-fsave-optimization-record`) it comes in the `Function` field, and in the
> diagnostics (`-Rpass*`) it uses debug info to show the source around it.
>
> if it's for testing only, you can test using the serialized remarks with YAML.
I followed the example from since it was brought up earlier.
https://github.com/llvm/llvm-project/blob/f40d25dd8d3ad7bcfa8f5e8f74f245ab1a7675df/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp#L1223
Also, there is not guarantee you have debug info when you run this, right? Also
you won't get a function name in the console if you run this over IR, even when
debug information is included w/in the IR.
I see `remark: <unknown>:0:0: ...` when running any of the IR tests.
================
Comment at: llvm/lib/CodeGen/StackFrameLayoutAnalysisPass.cpp:124
+ .str();
+ emitLayoutRemark(MF, "Stack Layout", "StackLayout", Slot);
+ }
----------------
thegameg wrote:
> We usually use identifiers for remark names, so here `StackLayout` instead of
> `Stack Layout`.
hmm, I was following the example in
https://github.com/llvm/llvm-project/blob/f40d25dd8d3ad7bcfa8f5e8f74f245ab1a7675df/llvm/lib/Target/AMDGPU/AMDGPUAsmPrinter.cpp#L1223,
but it looks like I may have swapped them. I'll take a closer look at the
output and fix accordingly (here and elsewhere).
================
Comment at: llvm/lib/CodeGen/StackFrameLayoutAnalysisPass.cpp:178
+
+ emitHeaderRemark(MF);
+
----------------
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...
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
