[PATCH] D46265: StackColoring: better handling of statically unreachable code

[Than McIntosh via Phabricator via llvm-commits]

thanm added a comment.

In https://reviews.llvm.org/D46265#1095398, @sdardis wrote:

> Sorry for the delay, but this change looks ok, but the supplied test case is flaky and crashes sometimes on my machine:
>
> Digging in with LLDB gives differing results (somewhat expected). Can you run valgrind on the run of llc for this test case and report your findings?


You are right, the testcase is indeed flaky.  I left out the "-start-before stack-coloring" in the llc run rule -- as a result, what's happening is that the UnreachableBlockElimLegacyPass is running (prior to stack coloring), so it deletes the unreachable BB "unreachable:". The corresponding unreachable machine BB still exists, however, and it includes a reference to the BB, which becomes stale, leading to the crash.

If I add back in "-start-before stack-coloring" to the LLC command line (so as not preserve the BBs), I am back at my original problem of

Unable to schedule 'Slot index numbering' required by 'Merge disjoint stack slots'

for which I have no viable solution. I think what I will do is add a new knob to disable unreachable block elimination, then run llc as usual. Let me work on that.



================
Comment at: test/CodeGen/X86/stack-coloring-unreachable-bb.mir:1
+# RUN: llc -mcpu=corei7 -no-stack-coloring=false -stop-after stack-coloring -o - %s
+
----------------
sdardis wrote:
> sdardis wrote:
> > Either rename this file to PR37310.mir or cite the PR number as a comment.
> Add -mtriple=x86_64-unknown-linux-gnu to the command line here.
Will do.


================
Comment at: test/CodeGen/X86/stack-coloring-unreachable-bb.mir:3
+
+# This MIR testcase was created by compiling the following test, first
+# with "clang -emit-llvm -S" and then "llc -stop-before stack-coloring",
----------------
sdardis wrote:
> Explain that this is testing that the stack coloring pass handles statically unreachable blocks.
Will do.


================
Comment at: test/CodeGen/X86/stack-coloring-unreachable-bb.mir:22-25
+  ; ModuleID = 'llvm-pr-37130.opt.ll'
+  source_filename = "llvm-pr-37130.c"
+  target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+  target triple = "x86_64-unknown-linux-gnu"
----------------
sdardis wrote:
> This can all be removed.
OK, will do.  Regarding this and your other suggestions, is there a tool or programmatic way to do this? Or is this just an expected part of creating an MIR testcase?  Please bear with me, I haven't written a lot of MIR tests.


================
Comment at: test/CodeGen/X86/stack-coloring-unreachable-bb.mir:28
+  ; Function Attrs: nounwind uwtable
+  define void @foo(i32 %x) local_unnamed_addr #0 {
+  entry:
----------------
sdardis wrote:
> local_unnamed_addr can be removed.
Will do.


================
Comment at: test/CodeGen/X86/stack-coloring-unreachable-bb.mir:69-72
+  attributes #0 = { nounwind uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+  attributes #1 = { argmemonly nounwind }
+  attributes #2 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="false" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="x86-64" "target-features"="+fxsr,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" "use-soft-float"="false" }
+  attributes #3 = { nounwind }
----------------
sdardis wrote:
> As far as I can see, these can be removed as well, along with #<num>s in the function declarations.
Will do.


================
Comment at: test/CodeGen/X86/stack-coloring-unreachable-bb.mir:73-78
+  
+  !llvm.module.flags = !{!0}
+  !llvm.ident = !{!1}
+  
+  !0 = !{i32 1, !"wchar_size", i32 4}
+  !1 = !{!"clang version 7.0.0 (trunk 331069) (llvm/trunk 331070)"}
----------------
sdardis wrote:
> This can be removed.
Will do.


Repository:
  rL LLVM

https://reviews.llvm.org/D46265