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

[Paul Kirth via Phabricator via cfe-commits]

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