[PATCH] D18827: Rework/enhance stack coloring data flow analysis.

[Wei Mi via llvm-commits]

wmi added a comment.

Hi Than,

It is a nice improvement and the code looks clean.

I can see the patch should work well for caller func with local vars introduced from callee during inline. The only concern I have is for independent local vars both defined in the same func: Because an interval is from use to @llvm.lifetime.end, even if two local vars have separate live ranges, they may still be regarded as overlapped only because their live intervals are extended from the last use to @llvm.lifetime.end. I understand it is safe to use @llvm.lifetime.end as the end of live interval, especically when there are indirect accesses to those local vars, but it can also limit the improvement we could have got, especially for local vars defined in the same func. I tried the motivational testcase stack-ifthen.cc and found it had the problem described here too - it still allocated space for both itar1 and itar2 (@llvm.lifetime.end of itar1 and itar2 are both sinked to the func exit, not at the end of then and else branches. I apply the patch and the command I use: ~/workarea/llvm-r265225/dbuild/bin/clang++ -O2 -S stack-ifthen.cc).

If we want to recognize more exact live interval of local vars, we need to involve alias analysis to count every alias access also as the uses of a local var in addition to direct use of local vars. Or at least if we know which local vars don't have the address saved or passed, we can only use their direct uses to compute live interval.

Since inline is an important source of increasing stack space, I think it maybe ok to leave the more exact interval requirement described here for further improvement. The cause that stack-ifthen.cc were not improved may be worthy to look at.

Other inline comments are nits.

Thanks,
Wei.


================
Comment at: lib/CodeGen/StackColoring.cpp:435
@@ -361,2 +434,3 @@
 
+      // Update block LiveOut set, noting whether it has changed
       if (LocalLiveOut.test(BlockInfo.LiveOut)) {
----------------
Since we only do dataflow iteration in forward direction now, if only no change in LiveIn, there shouldn't be change in LiveOut. So can we move BlockInfo.LiveOut |= LocalLiveOut into the above if?

================
Comment at: lib/CodeGen/StackColoring.cpp:809
@@ -716,2 +808,3 @@
   // Calculate the liveness of each block.
-  calculateLocalLiveness();
+  unsigned NumIters = calculateLocalLiveness();
+  DEBUG(dbgs()<< "Dataflow iterations: " << NumIters << "\n");
----------------
There may be an unused warning when NDEBUG is enabled.

================
Comment at: test/CodeGen/X86/StackColoring.ll:445
@@ +444,3 @@
+  store i32 %x, i32* %x.addr, align 4
+  %0 = bitcast [128 x i32]* %itar1 to i8*
+  call void @llvm.lifetime.start(i64 512, i8* %0) #3
----------------
%0, %1, %2 are prone to changing. we can run "opt -instnamer" to rename %0, %1, %2..., so the test will be stabler.


http://reviews.llvm.org/D18827