[PATCH] D115808: [DAGCombiner] Avoid combining adjacent stores at -O0 to improve debug experience

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/test/CodeGen/RISCV/dbg-combine.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s | FileCheck %s
----------------
dbg-combine isn't a great name for this simplified test, maybe something like optnone-store-no-combine?


================
Comment at: llvm/test/CodeGen/RISCV/dbg-combine.ll:4-6
+; Make sure that the sequence of store instructions for function foo is correctly
+; generated. More specifically, `sw a0, -20(s0)` instruction sequences must appear in
+; the output assembly.
----------------
(a) It's _correct_ to optimise out redundant stores, just unhelpful in your specific use case, so don't use that term here

(b) Specifying the exact instruction is a bad idea, you don't care what instruction it is and it could change in future (e.g. frame layout gets shifted about for some reason, or we change to using sp-relative rather than s0-relative), just that each store in the IR gets a corresponding store in the generated code rather than being optimised out


================
Comment at: llvm/test/CodeGen/RISCV/dbg-combine.ll:8-18
+; cat dbg-combine.c
+; 1 int main ()
+; 2 {
+; 3   int size;
+; 4
+; 5   size = (int) sizeof (long long);
+; 6   size = (int) sizeof (void*);
----------------
We don't tend to include C for IR tests, because the IR is supposed to have been distilled down to something simple and readable that can be understood on its own. Especially for a test as simple as this the C is a complete waste of space (and overly complicated with the use of `sizeof`, how the value came to be is totally irrelevant).


================
Comment at: llvm/test/CodeGen/RISCV/dbg-combine.ll:20
+
+define dso_local signext i32 @foo() #0 {
+; CHECK-LABEL: foo:
----------------
Return type is unnecessary, just use void


================
Comment at: llvm/test/CodeGen/RISCV/dbg-combine.ll:36
+; CHECK-NEXT:    ret
+entry:
+  %size = alloca i32, align 4
----------------
Don't think this is needed


================
Comment at: llvm/test/CodeGen/RISCV/dbg-combine.ll:37
+entry:
+  %size = alloca i32, align 4
+  store i32 8, i32* %size, align 4
----------------
Generally load/store tests will take a pointer as an argument to the function rather than complicating things with an alloca (i.e. `@foo(i32* %p)`)


================
Comment at: llvm/test/CodeGen/RISCV/dbg-combine.ll:44
+
+attributes #0 = { noinline nounwind optnone "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+64bit,+a,+c,+m,+relax,-save-restore" }
+
----------------
Way too complicated, all you care about is optnone (and _possibly_ nounwind depending on whether excluding it ends up cluttering the assembly with CFI directives), and those should both be specified inline rather than indirectly via `#0`


================
Comment at: llvm/test/CodeGen/RISCV/dbg-combine.ll:45
+attributes #0 = { noinline nounwind optnone "frame-pointer"="all" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="+64bit,+a,+c,+m,+relax,-save-restore" }
+
----------------
Trailing blank lines should go


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115808